From bef3c2938a6fbddb045112de57090fc6de60fa2e Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Mon, 31 Aug 2026 16:18:59 +0200 Subject: [PATCH 01/15] fix(policy): return 400 for invalid request bodies instead of 500 @Valid request-body validation failures and malformed JSON bodies were falling through to GlobalExceptionHandler's generic Exception handler, which returns 500. Add dedicated MethodArgumentNotValidException and HttpMessageNotReadableException handlers so both cases return 400, as already relied on by CertificateController's bootstrap endpoint and the new product discovery endpoint. --- .../handlers/GlobalExceptionHandler.java | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java index 4622403..42e5144 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java @@ -10,7 +10,9 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; import org.springframework.security.authorization.AuthorizationDeniedException; +import org.springframework.web.bind.MethodArgumentNotValidException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; import org.springframework.web.context.request.WebRequest; @@ -136,6 +138,53 @@ public ResponseEntity handlePkiException(PkiException ex, WebRequ return new ResponseEntity<>(errorResponse, HttpStatus.INTERNAL_SERVER_ERROR); } + /** + * Handles {@code @Valid} request body validation failures (e.g. field size/blank + * constraints) with a 400, rather than falling through to the 500 handler below. + * + * @param ex the exception + * @param request the current request + * @return a ResponseEntity with a 400 error message + */ + @ExceptionHandler(MethodArgumentNotValidException.class) + public ResponseEntity handleMethodArgumentNotValidException( + MethodArgumentNotValidException ex, WebRequest request) { + + String errorId = generateErrorId(); + log.debug( + "Request validation failed, error_id={}, path={}: {}", + errorId, + request.getContextPath(), + ex.getMessage()); + + ErrorResponse errorResponse = + new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Invalid request: " + ex.getMessage(), errorId); + + return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST); + } + + /** + * Handles malformed/unreadable request bodies (e.g. invalid JSON, wrong field types) + * with a 400, rather than falling through to the 500 handler below. + * + * @param ex the exception + * @param request the current request + * @return a ResponseEntity with a 400 error message + */ + @ExceptionHandler(HttpMessageNotReadableException.class) + public ResponseEntity handleHttpMessageNotReadableException( + HttpMessageNotReadableException ex, WebRequest request) { + + String errorId = generateErrorId(); + log.debug( + "Malformed request body, error_id={}, path={}: {}", errorId, request.getContextPath(), ex.getMessage()); + + ErrorResponse errorResponse = + new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Invalid request body", errorId); + + return new ResponseEntity<>(errorResponse, HttpStatus.BAD_REQUEST); + } + /** * Handles RuntimeException. * From 6812287baa83d781dbbda8a77735d9880569e2c6 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Mon, 31 Aug 2026 16:18:59 +0200 Subject: [PATCH 02/15] refactor(policy): expose organisation id lookup for reuse outside config Widen RequestRejectionSupport (and getOrganisationId) to public so the new product discovery controller, in a different package, can read the organisation CertificateValidationInterceptor already resolved for the request instead of duplicating the request-attribute lookup. --- .../node/management/config/RequestRejectionSupport.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java index af9b70f..761847d 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java @@ -19,9 +19,11 @@ /** * Shared request-rejection behaviour for {@code HandlerInterceptor}s that gate access * on the authenticated client: resolving the client id from the security context and - * writing a JSON {@link ErrorResponse} for a rejected request. + * writing a JSON {@link ErrorResponse} for a rejected request. {@link #getOrganisationId} + * is also read by controllers (e.g. product discovery) that need the organisation + * {@link CertificateValidationInterceptor} resolved for the current request. */ -final class RequestRejectionSupport { +public final class RequestRejectionSupport { private static final String ORGANISATION_ID_ATTRIBUTE = "ndtp.organisationId"; @@ -31,7 +33,7 @@ static void setOrganisationId(HttpServletRequest request, Long organisationId) { request.setAttribute(ORGANISATION_ID_ATTRIBUTE, organisationId); } - static String getOrganisationId(HttpServletRequest request) { + public static String getOrganisationId(HttpServletRequest request) { Object value = request.getAttribute(ORGANISATION_ID_ATTRIBUTE); return value == null ? null : String.valueOf(value); } From 8135d4c84b01a39bc97a6c5f166af0e5d020788f Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Mon, 31 Aug 2026 16:18:59 +0200 Subject: [PATCH 03/15] feat(product-discovery): add policy-aware product discovery endpoint Implements DPAV-3018: POST /api/v1/product/discovery returns only the products the authenticated requester is authorised to see. Search criteria (name/topic/type, all optional) narrow the org-unscoped candidate query, then ProductDiscoveryService evaluates one PDP decision per candidate via the existing PolicyDecisionClient (built for DPAV-3017's PEP), keeping only ALLOWed products - a PDP denial or failure excludes just that candidate rather than the whole request. - ProductRepository.findDiscoveryCandidates: bounded, filtered candidate query across all organisations (policy decides visibility, not org membership) - ProductDiscoveryService: queries candidates then filters by policy; new `discover_products` role gates the endpoint - docker/opa/policy.rego: starting example for a discover-action rule - application.yml: application.product-discovery.max-candidates bounds the per-request PDP call count --- docker/opa/policy.rego | 7 + docs/AUTHENTICATION_REQUIREMENTS.md | 5 + .../v1/ProductDiscoveryController.java | 88 +++++++++ .../model/dto/ProductDiscoveryRequestDTO.java | 36 ++++ .../dto/ProductDiscoveryResponseDTO.java | 31 +++ .../repository/ProductRepository.java | 25 +++ .../service/data/ProductDiscoveryService.java | 44 +++++ .../service/data/ProductService.java | 13 ++ .../impl/ProductDiscoveryServiceImpl.java | 71 +++++++ .../service/data/impl/ProductServiceImpl.java | 28 ++- .../service/providers/policy/PolicyInput.java | 10 +- src/main/resources/application.yml | 4 + .../v1/ProductDiscoveryControllerTest.java | 183 ++++++++++++++++++ .../model/dto/ProductDiscoveryDtoTest.java | 71 +++++++ .../impl/ProductDiscoveryServiceImplTest.java | 126 ++++++++++++ .../data/impl/ProductServiceImplTest.java | 51 ++++- 16 files changed, 787 insertions(+), 6 deletions(-) create mode 100644 docker/opa/policy.rego create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java diff --git a/docker/opa/policy.rego b/docker/opa/policy.rego new file mode 100644 index 0000000..005ab9f --- /dev/null +++ b/docker/opa/policy.rego @@ -0,0 +1,7 @@ +package management_node + +default allow = true + +# Product discovery (ProductDiscoveryService) evaluates one decision per candidate product, +# with resource "product:{id}" and action "discover" - see PolicyInput. A real discovery +# policy belongs here once authored (see docs/POLICY_ENFORCEMENT_TESTING.md). diff --git a/docs/AUTHENTICATION_REQUIREMENTS.md b/docs/AUTHENTICATION_REQUIREMENTS.md index b775d5a..b12cf82 100644 --- a/docs/AUTHENTICATION_REQUIREMENTS.md +++ b/docs/AUTHENTICATION_REQUIREMENTS.md @@ -78,6 +78,9 @@ Notes: - Bootstrap Certificate API: The onboarding service account may request bootstrap certificate packages when its token contains the role `request_bootstrap_certificate`. The request body contains the target `organisationId` and a CSR. If no certificate record exists for the organisation, one is created automatically. This role is typically assigned only to the website backend service account, not to individual federator clients. - Enforcement in code: `@PreAuthorize("hasAuthority('ROLE_management-node:request_bootstrap_certificate')")` on `POST /api/v1/certificate/bootstrap`. +- Product Discovery API: Clients may discover the products they are authorised to see when their token contains the role `discover_products`. Even with the role, results are further filtered per-product by the PDP (see `docs/POLICY_ENFORCEMENT_TESTING.md`) - the role only gates access to the endpoint itself. + - Enforcement in code: `@PreAuthorize("hasAuthority('ROLE_management-node:discover_products')")` on `POST /api/v1/product/discovery`. + ## How this maps to Keycloak - In Keycloak, roles are typically assigned to a client (here conceptually the `management-node` client) and appear in tokens under `resource_access["management-node"].roles`. @@ -91,6 +94,7 @@ Notes: - `sign_certificate` - `access_public_certificates` - `request_bootstrap_certificate` + - `discover_products` - Assign configuration roles to the appropriate Producer or Consumer Federator clients or service accounts. - Assign certificate roles (`create_keys`, `sign_certificate`, `access_public_certificates`) to federator service accounts that manage their own certificates. - Assign `request_bootstrap_certificate` only to the website/onboarding backend service account. @@ -123,4 +127,5 @@ curl -k 'https://localhost:8090/api/v1/configuration/producer' \ - CSR Signing API requires role: `sign_certificate`. - Intermediate Certificate API requires role: `access_public_certificates`. - Bootstrap Certificate API requires role: `request_bootstrap_certificate`. + - Product Discovery API requires role: `discover_products` (plus per-product PDP authorisation). - Swagger/OpenAPI: Use Swagger UI at `/swagger-ui.html` to explore and test with a valid token. \ No newline at end of file diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java new file mode 100644 index 0000000..9c2a96e --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java @@ -0,0 +1,88 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.controller.v1; + +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.Parameter; +import io.swagger.v3.oas.annotations.media.Content; +import io.swagger.v3.oas.annotations.media.Schema; +import io.swagger.v3.oas.annotations.responses.ApiResponse; +import io.swagger.v3.oas.annotations.security.SecurityRequirement; +import io.swagger.v3.oas.annotations.tags.Tag; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.validation.Valid; +import lombok.extern.slf4j.Slf4j; +import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.security.core.annotation.AuthenticationPrincipal; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; +import uk.gov.dbt.ndtp.ia.node.management.config.RequestRejectionSupport; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryRequestDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductDiscoveryService; + +@RestController +@RequestMapping("/api/v1/product") +@Slf4j +@Tag( + name = "Product Discovery", + description = "Policy-aware discovery of data products the requester is authorised to see.") +public class ProductDiscoveryController { + + private final ProductDiscoveryService productDiscoveryService; + + public ProductDiscoveryController(ProductDiscoveryService productDiscoveryService) { + this.productDiscoveryService = productDiscoveryService; + } + + @PostMapping("/discovery") + @PreAuthorize("hasAuthority('ROLE_management-node:discover_products')") + @Operation( + summary = "Discover authorised products", + description = "Returns only the products the authenticated requester is authorised to discover, " + + "narrowed by the supplied search criteria. Never returns products denied by policy, " + + "even if they match the search criteria.", + security = {@SecurityRequirement(name = "bearerAuth")}) + @ApiResponse( + responseCode = "200", + description = "Discovery response returned (possibly with an empty product list)", + content = + @Content( + mediaType = "application/json", + schema = @Schema(implementation = ProductDiscoveryResponseDTO.class))) + @ApiResponse(responseCode = "400", description = "Invalid request body") + @ApiResponse(responseCode = "401", description = "Unauthorized") + @ApiResponse(responseCode = "403", description = "Forbidden") + @ApiResponse(responseCode = "500", description = "Internal server error") + public ProductDiscoveryResponseDTO discoverProducts( + @Parameter(hidden = true) @AuthenticationPrincipal EnhancedPrincipal principal, + HttpServletRequest request, + @Valid @RequestBody(required = false) ProductDiscoveryRequestDTO criteria) { + ProductDiscoveryRequestDTO effectiveCriteria = criteria != null + ? criteria + : ProductDiscoveryRequestDTO.builder().build(); + String organisation = RequestRejectionSupport.getOrganisationId(request); + + log.info( + "Product discovery request clientId={} organisation={} name={} topic={} type={}", + principal.clientId(), + organisation, + effectiveCriteria.getName(), + effectiveCriteria.getTopic(), + effectiveCriteria.getType()); + + return productDiscoveryService.discover( + principal.clientId(), + organisation, + effectiveCriteria.getName(), + effectiveCriteria.getTopic(), + effectiveCriteria.getType()); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java new file mode 100644 index 0000000..262ee50 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java @@ -0,0 +1,36 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.model.dto; + +import jakarta.validation.constraints.Size; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; + +/** + * Search criteria for {@code POST /v1/product/discovery}. All fields are optional; an + * empty/absent field means "no filter" on that attribute. Filters only narrow the set of + * products the requester is authorised to discover - they cannot widen it. + */ +@Builder +@Getter +@Setter +@NoArgsConstructor +@AllArgsConstructor +public class ProductDiscoveryRequestDTO { + + @Size(max = 50) + private String name; + + @Size(max = 150) + private String topic; + + @Size(max = 255) + private String type; +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java new file mode 100644 index 0000000..285f007 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java @@ -0,0 +1,31 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.model.dto; + +import java.util.ArrayList; +import java.util.List; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; + +/** + * Response for {@code POST /v1/product/discovery}: the products the requester is authorised + * to discover, after policy filtering and search criteria are both applied. Empty (never + * null) when no products are authorised or none match the search criteria. + */ +@Builder +@Getter +@Setter +@NoArgsConstructor +@AllArgsConstructor +public class ProductDiscoveryResponseDTO { + + @Builder.Default + private List products = new ArrayList<>(); +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java index 2c74b10..70a8779 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java @@ -7,8 +7,10 @@ package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; import java.util.List; +import org.springframework.data.domain.Pageable; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.Query; +import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Product; @@ -41,4 +43,27 @@ public interface ProductRepository extends JpaRepository { */ @Query("SELECT o FROM Product o " + "JOIN FETCH o.productType t " + " WHERE o.producer.id IN :producers") List findByProducerIds(List producers); + + /** + * Discovery candidate query: products across all organisations matching the optional + * search filters (case-insensitive contains on name/topic, exact match on type name). + * A {@code null} filter matches everything for that attribute. Not organisation-scoped - + * policy (the PDP), not org membership, decides visibility for discovery. Uses a LEFT + * JOIN on productType (unlike the other queries here) since type is optional and a + * product without one must still be a candidate when no type filter is supplied. + * + * @param name optional case-insensitive contains filter on product name + * @param topic optional case-insensitive contains filter on product topic + * @param type optional case-insensitive exact filter on product type name + * @param pageable bounds the candidate set size (e.g. {@code PageRequest.of(0, maxCandidates)}) + * @return candidate products matching the filters, bounded by {@code pageable} + */ + @Query("SELECT p FROM Product p " + + "LEFT JOIN FETCH p.productType t " + + "WHERE (:name IS NULL OR LOWER(p.name) LIKE LOWER(CONCAT('%', :name, '%'))) " + + "AND (:topic IS NULL OR LOWER(p.topic) LIKE LOWER(CONCAT('%', :topic, '%'))) " + + "AND (:type IS NULL OR LOWER(t.name) = LOWER(:type)) " + + "ORDER BY p.id") + List findDiscoveryCandidates( + @Param("name") String name, @Param("topic") String topic, @Param("type") String type, Pageable pageable); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java new file mode 100644 index 0000000..c016a66 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java @@ -0,0 +1,44 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.data; + +import java.util.List; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; + +/** + * Runs product discovery: queries candidate products matching the requester's search + * criteria, then applies per-candidate PDP authorisation, keeping only the products the + * requester is authorised to discover. + */ +public interface ProductDiscoveryService { + + /** + * Queries discovery candidates matching the given search criteria, then evaluates one + * PDP decision per candidate, keeping only the ALLOWed ones. + * + * @param clientId identity of the calling client + * @param organisation organisation the client belongs to, if known + * @param name optional case-insensitive contains filter on product name + * @param topic optional case-insensitive contains filter on product topic + * @param type optional case-insensitive exact filter on product type name + * @return the products the requester is authorised to discover, matching the criteria + */ + ProductDiscoveryResponseDTO discover(String clientId, String organisation, String name, String topic, String type); + + /** + * Evaluates one PDP decision per candidate product and returns only the ALLOWed ones. A + * candidate is excluded (not the whole request failed) if the PDP denies it or the PDP + * call itself fails, so a partial PDP outage degrades results rather than the request. + * + * @param clientId identity of the calling client + * @param organisation organisation the client belongs to, if known + * @param candidates discovery candidate products to authorise + * @return the subset of candidates the PDP allows for this requester + */ + List filterAuthorised(String clientId, String organisation, List candidates); +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductService.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductService.java index 1507ed7..ccdb0d0 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductService.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductService.java @@ -29,4 +29,17 @@ public interface ProductService { * @return a list of DataProviderDTO objects corresponding to the given producer IDs */ List getProductsByProducerIds(List producerIds); + + /** + * Retrieves discovery candidate products across all organisations matching the optional + * search filters, bounded by the configured max-candidate limit. This is the pre-policy + * candidate set for {@code POST /v1/product/discovery}; authorisation is applied + * separately, per candidate, by the PDP. + * + * @param name optional case-insensitive contains filter on product name + * @param topic optional case-insensitive contains filter on product topic + * @param type optional case-insensitive exact filter on product type name + * @return candidate products matching the filters, bounded by the max-candidate limit + */ + List findDiscoveryCandidates(String name, String topic, String type); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java new file mode 100644 index 0000000..549f4e6 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java @@ -0,0 +1,71 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.data.impl; + +import java.util.List; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Service; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductDiscoveryService; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductService; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecision; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecisionClient; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyInput; + +/** + * Reuses {@link PolicyDecisionClient} (built for the whole-request PEP on + * {@code /api/v1/configuration/**}) once per candidate product, since discovery needs to + * authorise a set of resources rather than the single request URI. The {@code resource} and + * {@code action} fields of {@link PolicyInput} are repurposed here: {@code resource} carries + * a stable {@code PRODUCT_RESOURCE_PREFIX + id} identifier instead of a request URI, and + * {@code action} is the literal string {@code "discover"} instead of an HTTP method. + */ +@Service +@Slf4j +public class ProductDiscoveryServiceImpl implements ProductDiscoveryService { + + private static final String DISCOVER_ACTION = "discover"; + private static final String PRODUCT_RESOURCE_PREFIX = "product:"; + + private final ProductService productService; + private final PolicyDecisionClient policyDecisionClient; + + public ProductDiscoveryServiceImpl(ProductService productService, PolicyDecisionClient policyDecisionClient) { + this.productService = productService; + this.policyDecisionClient = policyDecisionClient; + } + + @Override + public ProductDiscoveryResponseDTO discover( + String clientId, String organisation, String name, String topic, String type) { + List candidates = productService.findDiscoveryCandidates(name, topic, type); + List authorised = filterAuthorised(clientId, organisation, candidates); + return ProductDiscoveryResponseDTO.builder().products(authorised).build(); + } + + @Override + public List filterAuthorised(String clientId, String organisation, List candidates) { + return candidates.stream() + .filter(candidate -> isAuthorised(clientId, organisation, candidate)) + .toList(); + } + + private boolean isAuthorised(String clientId, String organisation, ProductDTO candidate) { + PolicyInput input = + new PolicyInput(clientId, organisation, PRODUCT_RESOURCE_PREFIX + candidate.getId(), DISCOVER_ACTION); + PolicyDecision decision = policyDecisionClient.evaluate(input); + if (decision == PolicyDecision.DENY) { + log.debug( + "Policy decision DENY clientId={} resource={} action={}", + clientId, + input.resource(), + DISCOVER_ACTION); + } + return decision == PolicyDecision.ALLOW; + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java index 2762633..e36c0b9 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java @@ -8,7 +8,11 @@ import java.util.List; import java.util.Optional; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; import org.springframework.stereotype.Service; +import org.springframework.util.StringUtils; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ProductConverter; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Product; @@ -23,16 +27,23 @@ public class ProductServiceImpl implements ProductService { private final ProductRepository productRepository; private final ProductConverter productConverter; + private final int maxDiscoveryCandidates; /** * Constructor-based dependency injection. * * @param productRepository the organisation data provider repository * @param productConverter the converter for entity-to-DTO conversion + * @param maxDiscoveryCandidates upper bound on candidates fetched for discovery, keeping + * the per-candidate PDP call loop in {@code ProductDiscoveryService} bounded */ - public ProductServiceImpl(ProductRepository productRepository, ProductConverter productConverter) { + public ProductServiceImpl( + ProductRepository productRepository, + ProductConverter productConverter, + @Value("${application.product-discovery.max-candidates:200}") int maxDiscoveryCandidates) { this.productRepository = productRepository; this.productConverter = productConverter; + this.maxDiscoveryCandidates = maxDiscoveryCandidates; } /** @@ -59,4 +70,19 @@ public List getProductsByProducerIds(List producerIds) { .map(productConverter::toDtoList) .orElse(List.of()); } + + /** + * {@inheritDoc} + */ + @Override + public List findDiscoveryCandidates(String name, String topic, String type) { + Pageable limit = PageRequest.of(0, maxDiscoveryCandidates); + List candidates = productRepository.findDiscoveryCandidates( + blankToNull(name), blankToNull(topic), blankToNull(type), limit); + return productConverter.toDtoList(candidates); + } + + private static String blankToNull(String value) { + return StringUtils.hasText(value) ? value : null; + } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java index 4c24271..ed22c89 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java @@ -10,12 +10,16 @@ /** * Policy attributes describing who is making a request and what they are trying to do, - * sent to the PDP (OPA) as the {@code input} of a decision request. + * sent to the PDP (OPA) as the {@code input} of a decision request. {@code resource} and + * {@code action} are opaque strings whose convention is caller-defined: the whole-request + * PEP ({@link uk.gov.dbt.ndtp.ia.node.management.config.PolicyEnforcementInterceptor}) uses + * the request URI and HTTP method; per-candidate callers (e.g. product discovery) may use a + * different convention, such as a stable resource id and a named action. * * @param clientId identity of the calling client * @param organisation organisation the client belongs to, if known - * @param resource the requested resource (request URI) - * @param action the requested action (HTTP method) + * @param resource the resource being evaluated, in whatever convention the caller uses + * @param action the action being evaluated, in whatever convention the caller uses */ @JsonInclude(JsonInclude.Include.NON_NULL) public record PolicyInput(String clientId, String organisation, String resource, String action) {} diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml index 9276141..81c18fa 100644 --- a/src/main/resources/application.yml +++ b/src/main/resources/application.yml @@ -69,6 +69,10 @@ application: read-timeout: ${OPA_READ_TIMEOUT:3s} # max time to wait for an OPA decision response protected-paths: # API path patterns the Policy Enforcement Point intercepts - /api/v1/configuration/** + product-discovery: + # upper bound on candidates fetched per discovery request, before per-candidate PDP + # evaluation - keeps the synchronous PDP call loop bounded + max-candidates: ${PRODUCT_DISCOVERY_MAX_CANDIDATES:200} # Actuator Configuration management: diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java new file mode 100644 index 0000000..b3c6d4f --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java @@ -0,0 +1,183 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.controller.v1; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.MediaType; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContext; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.web.method.annotation.AuthenticationPrincipalArgumentResolver; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import uk.gov.dbt.ndtp.ia.node.management.exception.handlers.GlobalExceptionHandler; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductDiscoveryService; + +/** + * Integration test for {@code POST /api/v1/product/discovery} wiring + * {@link ProductDiscoveryController} to a mocked {@link ProductDiscoveryService}, covering + * the discovery spec scenarios (fully permitted, partially filtered, no candidates, no + * authorised products, and request validation, AC1-AC9). + */ +@ExtendWith(MockitoExtension.class) +class ProductDiscoveryControllerTest { + + @Mock + private ProductDiscoveryService productDiscoveryService; + + private MockMvc mockMvc; + + @BeforeEach + void setUp() { + ProductDiscoveryController controller = new ProductDiscoveryController(productDiscoveryService); + mockMvc = MockMvcBuilders.standaloneSetup(controller) + .setControllerAdvice(new GlobalExceptionHandler()) + .setCustomArgumentResolvers(new AuthenticationPrincipalArgumentResolver()) + .build(); + authenticateAs("client-1"); + } + + @AfterEach + void tearDown() { + SecurityContextHolder.clearContext(); + } + + private void authenticateAs(String clientId) { + // lenient: not every test (e.g. request-validation-failure tests) reaches argument + // resolution far enough to consult these mocks + EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId); + Authentication authentication = mock(Authentication.class); + lenient().when(authentication.getPrincipal()).thenReturn(principal); + SecurityContext context = mock(SecurityContext.class); + lenient().when(context.getAuthentication()).thenReturn(authentication); + SecurityContextHolder.setContext(context); + } + + private static ProductDiscoveryResponseDTO responseWith(ProductDTO... products) { + return ProductDiscoveryResponseDTO.builder().products(List.of(products)).build(); + } + + @Test + void fullyPermitted_returnsAllCandidates() throws Exception { + ProductDTO product = ProductDTO.builder().id(1L).name("Alpha").build(); + when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + .thenReturn(responseWith(product)); + + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.products.length()").value(1)) + .andExpect(jsonPath("$.products[0].name").value("Alpha")); + } + + @Test + void partiallyFiltered_returnsOnlyAuthorisedSubset() throws Exception { + ProductDTO allowed = ProductDTO.builder().id(1L).name("Allowed").build(); + when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + .thenReturn(responseWith(allowed)); + + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.products.length()").value(1)) + .andExpect(jsonPath("$.products[0].name").value("Allowed")); + } + + @Test + void noCandidates_returnsEmptyListNotError() throws Exception { + when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + .thenReturn(responseWith()); + + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.products").isEmpty()); + } + + @Test + void noAuthorisedProducts_returnsEmptyListNotError() throws Exception { + when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + .thenReturn(responseWith()); + + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.products").isEmpty()); + } + + @Test + void filterMatchingDeniedProduct_stillExcludedFromResponse() throws Exception { + // A search filter matching a product does not widen what the PDP authorises: the + // candidate query narrows by filter, but the PDP filter (mocked here as denying it) + // still wins. + when(productDiscoveryService.discover(anyString(), any(), eq("Restricted"), any(), any())) + .thenReturn(responseWith()); + + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"name\":\"Restricted\"}")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.products").isEmpty()); + } + + @Test + void invalidRequestBody_oversizedField_returns400() throws Exception { + String oversizedName = "x".repeat(51); + + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"name\":\"" + oversizedName + "\"}")) + .andExpect(status().isBadRequest()); + + verifyNoInteractions(productDiscoveryService); + } + + @Test + void malformedJsonBody_returns400() throws Exception { + mockMvc.perform(post("/api/v1/product/discovery") + .contentType(MediaType.APPLICATION_JSON) + .content("{not-json")) + .andExpect(status().isBadRequest()); + + verifyNoInteractions(productDiscoveryService); + } + + @Test + void emptyBody_treatedAsNoFilter() throws Exception { + when(productDiscoveryService.discover(anyString(), any(), isNull(), isNull(), isNull())) + .thenReturn(responseWith()); + + mockMvc.perform(post("/api/v1/product/discovery").contentType(MediaType.APPLICATION_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.products").isEmpty()); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java new file mode 100644 index 0000000..867cdb3 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java @@ -0,0 +1,71 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.model.dto; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.validation.ConstraintViolation; +import jakarta.validation.Validation; +import jakarta.validation.Validator; +import jakarta.validation.ValidatorFactory; +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.Test; + +class ProductDiscoveryDtoTest { + + private final ObjectMapper objectMapper = new ObjectMapper(); + + @Test + void requestDTO_emptyObject_deserializesWithNoViolations() throws Exception { + ProductDiscoveryRequestDTO dto = objectMapper.readValue("{}", ProductDiscoveryRequestDTO.class); + + try (ValidatorFactory factory = Validation.buildDefaultValidatorFactory()) { + Validator validator = factory.getValidator(); + Set> violations = validator.validate(dto); + assertThat(violations).isEmpty(); + } + assertThat(dto.getName()).isNull(); + assertThat(dto.getTopic()).isNull(); + assertThat(dto.getType()).isNull(); + } + + @Test + void requestDTO_oversizedField_failsValidation() { + ProductDiscoveryRequestDTO dto = + ProductDiscoveryRequestDTO.builder().name("x".repeat(51)).build(); + + try (ValidatorFactory factory = Validation.buildDefaultValidatorFactory()) { + Validator validator = factory.getValidator(); + Set> violations = validator.validate(dto); + assertThat(violations).isNotEmpty(); + } + } + + @Test + void responseDTO_defaultsToEmptyList_notNull() throws Exception { + ProductDiscoveryResponseDTO dto = ProductDiscoveryResponseDTO.builder().build(); + + assertThat(dto.getProducts()).isNotNull().isEmpty(); + + String json = objectMapper.writeValueAsString(dto); + assertThat(json).contains("\"products\":[]"); + } + + @Test + void responseDTO_withProducts_serializesWithoutInternalId() throws Exception { + ProductDTO product = + ProductDTO.builder().id(99L).name("Alpha").topic("topic-1").build(); + ProductDiscoveryResponseDTO dto = + ProductDiscoveryResponseDTO.builder().products(List.of(product)).build(); + + String json = objectMapper.writeValueAsString(dto); + + assertThat(json).contains("\"name\":\"Alpha\"").doesNotContain("99"); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java new file mode 100644 index 0000000..eb815d8 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java @@ -0,0 +1,126 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.data.impl; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.List; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; +import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductService; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecision; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecisionClient; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyInput; + +/** + * Verifies {@link ProductDiscoveryServiceImpl} queries candidates then evaluates one PDP + * decision per candidate, keeping only ALLOWed products (fully permitted, partially + * permitted, and PDP-failure scenarios, AC3/AC4/AC9), and that no denied/failed candidate's + * data leaks into the result. + */ +@ExtendWith(MockitoExtension.class) +class ProductDiscoveryServiceImplTest { + + @Mock + private ProductService productService; + + @Mock + private PolicyDecisionClient policyDecisionClient; + + @InjectMocks + private ProductDiscoveryServiceImpl productDiscoveryService; + + private ProductDTO allowedProduct; + private ProductDTO deniedProduct; + + @BeforeEach + void setUp() { + allowedProduct = ProductDTO.builder().id(1L).name("Allowed").build(); + deniedProduct = ProductDTO.builder().id(2L).name("Denied").build(); + } + + @Test + void filterAuthorised_fullyPermitted_returnsAllCandidates() { + when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.ALLOW); + + List result = + productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct, deniedProduct)); + + assertThat(result).containsExactlyInAnyOrder(allowedProduct, deniedProduct); + } + + @Test + void filterAuthorised_partiallyPermitted_returnsOnlyAllowedAndLeaksNoDeniedData() { + // Also covers the PDP-failure case: PolicyDecisionClient already fails closed + // (returns DENY) on any PDP error, so a denied candidate here is indistinguishable + // from a failed one - both are excluded the same way. + when(policyDecisionClient.evaluate(argThatResource("product:1"))).thenReturn(PolicyDecision.ALLOW); + when(policyDecisionClient.evaluate(argThatResource("product:2"))).thenReturn(PolicyDecision.DENY); + + List result = + productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct, deniedProduct)); + + assertThat(result).containsExactly(allowedProduct); + assertThat(result).extracting(ProductDTO::getId).doesNotContain(2L); + assertThat(result).extracting(ProductDTO::getName).doesNotContain("Denied"); + } + + @Test + void filterAuthorised_noneAuthorised_returnsEmptyList() { + when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.DENY); + + List result = + productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct, deniedProduct)); + + assertThat(result).isEmpty(); + } + + @Test + void filterAuthorised_buildsPolicyInputWithDiscoverActionAndProductResource() { + when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.ALLOW); + + productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct)); + + verify(policyDecisionClient).evaluate(eq(new PolicyInput("client-1", "org-1", "product:1", "discover"))); + } + + @Test + void discover_queriesCandidatesThenFiltersByPolicy() { + when(productService.findDiscoveryCandidates("Alpha", "topic-1", "TypeA")) + .thenReturn(List.of(allowedProduct, deniedProduct)); + when(policyDecisionClient.evaluate(argThatResource("product:1"))).thenReturn(PolicyDecision.ALLOW); + when(policyDecisionClient.evaluate(argThatResource("product:2"))).thenReturn(PolicyDecision.DENY); + + ProductDiscoveryResponseDTO result = + productDiscoveryService.discover("client-1", "org-1", "Alpha", "topic-1", "TypeA"); + + assertThat(result.getProducts()).containsExactly(allowedProduct); + } + + @Test + void discover_noCandidates_returnsEmptyResponse() { + when(productService.findDiscoveryCandidates(any(), any(), any())).thenReturn(List.of()); + + ProductDiscoveryResponseDTO result = productDiscoveryService.discover("client-1", "org-1", null, null, null); + + assertThat(result.getProducts()).isEmpty(); + } + + private PolicyInput argThatResource(String resource) { + return org.mockito.ArgumentMatchers.argThat(input -> input != null && resource.equals(input.resource())); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java index 3917ed2..560d8f2 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java @@ -7,6 +7,10 @@ package uk.gov.dbt.ndtp.ia.node.management.service.data.impl; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.*; import java.util.Collections; @@ -14,9 +18,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.data.domain.Pageable; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ProductConverter; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Producer; @@ -32,7 +36,6 @@ class ProductServiceImplTest { @Mock private ProductConverter productConverter; - @InjectMocks private ProductServiceImpl productService; private Product product; @@ -43,6 +46,10 @@ class ProductServiceImplTest { @BeforeEach void setUp() { + // Constructed manually (not @InjectMocks) - the constructor's int max-candidates + // parameter has no mock to inject + productService = new ProductServiceImpl(productRepository, productConverter, 200); + // Set up test data Producer producer = new Producer(); producer.setId(producerId); @@ -197,4 +204,44 @@ void getProductsByProducerIds_withNullRepositoryResult_shouldReturnEmptyList() { verify(productRepository).findByProducerIds(producerIds); verify(productConverter, never()).toDtoList(any()); } + + @Test + void findDiscoveryCandidates_delegatesFiltersAndLimitToRepository() { + // Arrange: constructed directly (not @InjectMocks) so the max-candidates limit is explicit + ProductServiceImpl service = new ProductServiceImpl(productRepository, productConverter, 5); + List products = List.of(product); + List productDTOs = List.of(productDTO); + + when(productRepository.findDiscoveryCandidates(eq("Alpha"), eq("topic-1"), eq("TypeA"), any(Pageable.class))) + .thenReturn(products); + when(productConverter.toDtoList(products)).thenReturn(productDTOs); + + // Act + List result = service.findDiscoveryCandidates("Alpha", "topic-1", "TypeA"); + + // Assert + assertEquals(productDTOs, result); + verify(productRepository) + .findDiscoveryCandidates( + eq("Alpha"), + eq("topic-1"), + eq("TypeA"), + argThat(pageable -> pageable.getPageSize() == 5 && pageable.getPageNumber() == 0)); + } + + @Test + void findDiscoveryCandidates_blankFilters_passedAsNullToRepository() { + // Arrange + ProductServiceImpl service = new ProductServiceImpl(productRepository, productConverter, 5); + when(productRepository.findDiscoveryCandidates(isNull(), isNull(), isNull(), any(Pageable.class))) + .thenReturn(Collections.emptyList()); + when(productConverter.toDtoList(Collections.emptyList())).thenReturn(Collections.emptyList()); + + // Act + List result = service.findDiscoveryCandidates("", null, " "); + + // Assert + assertTrue(result.isEmpty()); + verify(productRepository).findDiscoveryCandidates(isNull(), isNull(), isNull(), any(Pageable.class)); + } } From e0ff3cbc3184a33c376a03d18565a6974d254cb0 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Mon, 31 Aug 2026 19:15:32 +0200 Subject: [PATCH 04/15] test(product-discovery): de-duplicate identical controller test bodies noAuthorisedProducts_returnsEmptyListNotError was byte-identical to noCandidates_returnsEmptyListNotError (Sonar). Give it distinct value: send search criteria and verify they're passed through to ProductDiscoveryService.discover unchanged, instead of repeating the same empty-body/empty-response assertion. --- .../controller/v1/ProductDiscoveryControllerTest.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java index b3c6d4f..40ac4d6 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java @@ -12,6 +12,7 @@ import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; @@ -123,15 +124,17 @@ void noCandidates_returnsEmptyListNotError() throws Exception { } @Test - void noAuthorisedProducts_returnsEmptyListNotError() throws Exception { + void noAuthorisedProducts_returnsEmptyListNotErrorAndPassesCriteriaThrough() throws Exception { when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) .thenReturn(responseWith()); mockMvc.perform(post("/api/v1/product/discovery") .contentType(MediaType.APPLICATION_JSON) - .content("{}")) + .content("{\"name\":\"Alpha\",\"topic\":\"topic-1\",\"type\":\"TypeA\"}")) .andExpect(status().isOk()) .andExpect(jsonPath("$.products").isEmpty()); + + verify(productDiscoveryService).discover(eq("client-1"), any(), eq("Alpha"), eq("topic-1"), eq("TypeA")); } @Test From 997f8e17c35db82fdb2978268f20412b617a876d Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Mon, 31 Aug 2026 19:15:35 +0200 Subject: [PATCH 05/15] test(product-discovery): remove useless eq() around sole verify argument evaluate() takes a single argument, so wrapping it in eq(...) is a no-op Mockito already does by default (Sonar). Pass the PolicyInput value directly. --- .../service/data/impl/ProductDiscoveryServiceImplTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java index eb815d8..698b611 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java @@ -8,7 +8,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -95,7 +94,7 @@ void filterAuthorised_buildsPolicyInputWithDiscoverActionAndProductResource() { productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct)); - verify(policyDecisionClient).evaluate(eq(new PolicyInput("client-1", "org-1", "product:1", "discover"))); + verify(policyDecisionClient).evaluate(new PolicyInput("client-1", "org-1", "product:1", "discover")); } @Test From 21a109efdbd5b869aa544a07d825906e70edc53f Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Tue, 1 Sep 2026 10:26:51 +0200 Subject: [PATCH 06/15] fix(product-discovery): log request URI, not empty context path, on 400s The MethodArgumentNotValidException/HttpMessageNotReadableException handlers logged request.getContextPath() (empty for a root-mapped app) instead of request.getDescription(false), unlike every sibling handler in this class - leaving debug logs with no indication of which endpoint a validation failure came from. --- .../exception/handlers/GlobalExceptionHandler.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java index 42e5144..f0ca438 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/exception/handlers/GlobalExceptionHandler.java @@ -154,7 +154,7 @@ public ResponseEntity handleMethodArgumentNotValidException( log.debug( "Request validation failed, error_id={}, path={}: {}", errorId, - request.getContextPath(), + request.getDescription(false), ex.getMessage()); ErrorResponse errorResponse = @@ -177,7 +177,10 @@ public ResponseEntity handleHttpMessageNotReadableException( String errorId = generateErrorId(); log.debug( - "Malformed request body, error_id={}, path={}: {}", errorId, request.getContextPath(), ex.getMessage()); + "Malformed request body, error_id={}, path={}: {}", + errorId, + request.getDescription(false), + ex.getMessage()); ErrorResponse errorResponse = new ErrorResponse(HttpStatus.BAD_REQUEST.value(), "Invalid request body", errorId); From 2c47062872e5529e5da4ab6536b8f6b74a688aa4 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Tue, 1 Sep 2026 10:26:55 +0200 Subject: [PATCH 07/15] fix(product-discovery): fail fast on invalid max-candidates config application.product-discovery.max-candidates fed straight into PageRequest.of(0, n), which throws IllegalArgumentException for n < 1 (e.g. a misconfigured 0, or an attempt to mean "unlimited") - crashing every discovery request with a 500 instead of failing at startup. Validate in the constructor, matching the sibling OpaProperties' fail-fast intent. --- .../service/data/impl/ProductServiceImpl.java | 7 +++++++ .../service/data/impl/ProductServiceImplTest.java | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java index e36c0b9..9654ffa 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImpl.java @@ -36,11 +36,18 @@ public class ProductServiceImpl implements ProductService { * @param productConverter the converter for entity-to-DTO conversion * @param maxDiscoveryCandidates upper bound on candidates fetched for discovery, keeping * the per-candidate PDP call loop in {@code ProductDiscoveryService} bounded + * @throws IllegalArgumentException if maxDiscoveryCandidates is less than 1 - fails fast + * at startup rather than on every discovery request (PageRequest.of rejects a page + * size below 1) */ public ProductServiceImpl( ProductRepository productRepository, ProductConverter productConverter, @Value("${application.product-discovery.max-candidates:200}") int maxDiscoveryCandidates) { + if (maxDiscoveryCandidates < 1) { + throw new IllegalArgumentException( + "application.product-discovery.max-candidates must be at least 1, got " + maxDiscoveryCandidates); + } this.productRepository = productRepository; this.productConverter = productConverter; this.maxDiscoveryCandidates = maxDiscoveryCandidates; diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java index 560d8f2..b7dd622 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductServiceImplTest.java @@ -244,4 +244,16 @@ void findDiscoveryCandidates_blankFilters_passedAsNullToRepository() { assertTrue(result.isEmpty()); verify(productRepository).findDiscoveryCandidates(isNull(), isNull(), isNull(), any(Pageable.class)); } + + @Test + void constructor_rejectsZeroMaxCandidates() { + assertThrows( + IllegalArgumentException.class, () -> new ProductServiceImpl(productRepository, productConverter, 0)); + } + + @Test + void constructor_rejectsNegativeMaxCandidates() { + assertThrows( + IllegalArgumentException.class, () -> new ProductServiceImpl(productRepository, productConverter, -1)); + } } From d7141d93899c7580a8fb4c19f3999fd7dde5a520 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Tue, 1 Sep 2026 10:26:59 +0200 Subject: [PATCH 08/15] fix(product-discovery): escape LIKE wildcards in name/topic filters findDiscoveryCandidates built its %contains% pattern via CONCAT without escaping the SQL LIKE metacharacters % and _ in the caller-supplied value, so e.g. name=Data_Feed also matched DataXFeed (since '_' is the LIKE single-char wildcard) - silently violating the documented "contains" filter contract for any name/topic containing % or _. --- .../repository/ProductRepository.java | 35 ++++++++-- .../repository/ProductRepositoryTest.java | 70 +++++++++++++++++++ 2 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java index 70a8779..359966a 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepository.java @@ -58,12 +58,39 @@ public interface ProductRepository extends JpaRepository { * @param pageable bounds the candidate set size (e.g. {@code PageRequest.of(0, maxCandidates)}) * @return candidate products matching the filters, bounded by {@code pageable} */ + default List findDiscoveryCandidates(String name, String topic, String type, Pageable pageable) { + return findDiscoveryCandidatesByPattern(containsPattern(name), containsPattern(topic), type, pageable); + } + + /** + * Backing query for {@link #findDiscoveryCandidates}. Takes pre-built, LIKE-escaped + * {@code %pattern%} strings (see {@link #containsPattern}) rather than raw filter values, + * so the LIKE wildcards {@code %}/{@code _} in caller-supplied input are matched + * literally, not interpreted as wildcards. + */ @Query("SELECT p FROM Product p " + "LEFT JOIN FETCH p.productType t " - + "WHERE (:name IS NULL OR LOWER(p.name) LIKE LOWER(CONCAT('%', :name, '%'))) " - + "AND (:topic IS NULL OR LOWER(p.topic) LIKE LOWER(CONCAT('%', :topic, '%'))) " + + "WHERE (:namePattern IS NULL OR LOWER(p.name) LIKE LOWER(:namePattern) ESCAPE '\\') " + + "AND (:topicPattern IS NULL OR LOWER(p.topic) LIKE LOWER(:topicPattern) ESCAPE '\\') " + "AND (:type IS NULL OR LOWER(t.name) = LOWER(:type)) " + "ORDER BY p.id") - List findDiscoveryCandidates( - @Param("name") String name, @Param("topic") String topic, @Param("type") String type, Pageable pageable); + List findDiscoveryCandidatesByPattern( + @Param("namePattern") String namePattern, + @Param("topicPattern") String topicPattern, + @Param("type") String type, + Pageable pageable); + + /** + * Builds a {@code %value%} LIKE pattern with the LIKE metacharacters {@code \}, {@code %} + * and {@code _} in {@code value} escaped (backslash-escaped, matching the query's + * {@code ESCAPE '\'} clause), so a search value containing them is matched literally + * instead of as wildcards. + */ + private static String containsPattern(String value) { + if (value == null) { + return null; + } + String escaped = value.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_"); + return "%" + escaped + "%"; + } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java new file mode 100644 index 0000000..ad8d2cb --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java @@ -0,0 +1,70 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.List; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Product; + +/** + * Verifies {@link ProductRepository#findDiscoveryCandidates}'s default method builds + * LIKE-escaped patterns before delegating to {@link ProductRepository#findDiscoveryCandidatesByPattern}, + * so a search value containing the LIKE metacharacters {@code %}/{@code _} is matched + * literally rather than as a wildcard. Mocked with {@code CALLS_REAL_METHODS} so the default + * method itself executes, with only the underlying {@code @Query} method stubbed. + */ +class ProductRepositoryTest { + + private final ProductRepository productRepository = + Mockito.mock(ProductRepository.class, Mockito.withSettings().defaultAnswer(Mockito.CALLS_REAL_METHODS)); + + @Test + void findDiscoveryCandidates_escapesPercentAndUnderscoreInNameAndTopic() { + Pageable pageable = PageRequest.of(0, 10); + when(productRepository.findDiscoveryCandidatesByPattern(any(), any(), any(), eq(pageable))) + .thenReturn(List.of()); + + productRepository.findDiscoveryCandidates("Data_Feed", "topic%1", "TypeA", pageable); + + verify(productRepository) + .findDiscoveryCandidatesByPattern(eq("%Data\\_Feed%"), eq("%topic\\%1%"), eq("TypeA"), eq(pageable)); + } + + @Test + void findDiscoveryCandidates_escapesLiteralBackslash() { + Pageable pageable = PageRequest.of(0, 10); + when(productRepository.findDiscoveryCandidatesByPattern(any(), any(), any(), eq(pageable))) + .thenReturn(List.of()); + + productRepository.findDiscoveryCandidates("a\\b", null, null, pageable); + + verify(productRepository).findDiscoveryCandidatesByPattern(eq("%a\\\\b%"), isNull(), isNull(), eq(pageable)); + } + + @Test + void findDiscoveryCandidates_nullFilters_passedAsNullPatterns() { + Pageable pageable = PageRequest.of(0, 10); + List expected = List.of(); + when(productRepository.findDiscoveryCandidatesByPattern(isNull(), isNull(), isNull(), eq(pageable))) + .thenReturn(expected); + + List result = productRepository.findDiscoveryCandidates(null, null, null, pageable); + + assertThat(result).isEqualTo(expected); + verify(productRepository).findDiscoveryCandidatesByPattern(isNull(), isNull(), isNull(), eq(pageable)); + } +} From 43d36f979cec7198ba59a7ee9a59e7dc1056a992 Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Wed, 2 Sep 2026 13:32:36 +0200 Subject: [PATCH 09/15] refactor(product-discovery): use records for the discovery DTOs Convert ProductDiscoveryRequestDTO/ResponseDTO from mutable Lombok classes to records - value objects with no post-construction mutation needs, so records fit better than the mutable builder pattern used elsewhere in model/dto. @Builder still works on records (Lombok >=1.18.30, this repo is on 1.18.46), so callers keep the same .builder()...build() API; only the getX() accessors change to the record's x() accessors. ProductDiscoveryResponseDTO's never-null products guarantee moves from @Builder.Default (builder-only) to a compact constructor (applies to every construction path, including Jackson deserialization). Matches the existing PolicyInput/ PolicyDecisionRequest/PolicyDecisionResponse record precedent from DPAV-3017. --- .../v1/ProductDiscoveryController.java | 12 +++++------ .../model/dto/ProductDiscoveryRequestDTO.java | 21 ++----------------- .../dto/ProductDiscoveryResponseDTO.java | 16 ++++---------- .../model/dto/ProductDiscoveryDtoTest.java | 8 +++---- .../impl/ProductDiscoveryServiceImplTest.java | 4 ++-- 5 files changed, 18 insertions(+), 43 deletions(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java index 9c2a96e..eb67dd3 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java @@ -74,15 +74,15 @@ public ProductDiscoveryResponseDTO discoverProducts( "Product discovery request clientId={} organisation={} name={} topic={} type={}", principal.clientId(), organisation, - effectiveCriteria.getName(), - effectiveCriteria.getTopic(), - effectiveCriteria.getType()); + effectiveCriteria.name(), + effectiveCriteria.topic(), + effectiveCriteria.type()); return productDiscoveryService.discover( principal.clientId(), organisation, - effectiveCriteria.getName(), - effectiveCriteria.getTopic(), - effectiveCriteria.getType()); + effectiveCriteria.name(), + effectiveCriteria.topic(), + effectiveCriteria.type()); } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java index 262ee50..1218bd0 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryRequestDTO.java @@ -7,11 +7,7 @@ package uk.gov.dbt.ndtp.ia.node.management.model.dto; import jakarta.validation.constraints.Size; -import lombok.AllArgsConstructor; import lombok.Builder; -import lombok.Getter; -import lombok.NoArgsConstructor; -import lombok.Setter; /** * Search criteria for {@code POST /v1/product/discovery}. All fields are optional; an @@ -19,18 +15,5 @@ * products the requester is authorised to discover - they cannot widen it. */ @Builder -@Getter -@Setter -@NoArgsConstructor -@AllArgsConstructor -public class ProductDiscoveryRequestDTO { - - @Size(max = 50) - private String name; - - @Size(max = 150) - private String topic; - - @Size(max = 255) - private String type; -} +public record ProductDiscoveryRequestDTO( + @Size(max = 50) String name, @Size(max = 150) String topic, @Size(max = 255) String type) {} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java index 285f007..5abf82f 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryResponseDTO.java @@ -8,11 +8,7 @@ import java.util.ArrayList; import java.util.List; -import lombok.AllArgsConstructor; import lombok.Builder; -import lombok.Getter; -import lombok.NoArgsConstructor; -import lombok.Setter; /** * Response for {@code POST /v1/product/discovery}: the products the requester is authorised @@ -20,12 +16,8 @@ * null) when no products are authorised or none match the search criteria. */ @Builder -@Getter -@Setter -@NoArgsConstructor -@AllArgsConstructor -public class ProductDiscoveryResponseDTO { - - @Builder.Default - private List products = new ArrayList<>(); +public record ProductDiscoveryResponseDTO(List products) { + public ProductDiscoveryResponseDTO { + products = products != null ? products : new ArrayList<>(); + } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java index 867cdb3..1bd251a 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDiscoveryDtoTest.java @@ -30,9 +30,9 @@ void requestDTO_emptyObject_deserializesWithNoViolations() throws Exception { Set> violations = validator.validate(dto); assertThat(violations).isEmpty(); } - assertThat(dto.getName()).isNull(); - assertThat(dto.getTopic()).isNull(); - assertThat(dto.getType()).isNull(); + assertThat(dto.name()).isNull(); + assertThat(dto.topic()).isNull(); + assertThat(dto.type()).isNull(); } @Test @@ -51,7 +51,7 @@ void requestDTO_oversizedField_failsValidation() { void responseDTO_defaultsToEmptyList_notNull() throws Exception { ProductDiscoveryResponseDTO dto = ProductDiscoveryResponseDTO.builder().build(); - assertThat(dto.getProducts()).isNotNull().isEmpty(); + assertThat(dto.products()).isNotNull().isEmpty(); String json = objectMapper.writeValueAsString(dto); assertThat(json).contains("\"products\":[]"); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java index 698b611..c0f3615 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java @@ -107,7 +107,7 @@ void discover_queriesCandidatesThenFiltersByPolicy() { ProductDiscoveryResponseDTO result = productDiscoveryService.discover("client-1", "org-1", "Alpha", "topic-1", "TypeA"); - assertThat(result.getProducts()).containsExactly(allowedProduct); + assertThat(result.products()).containsExactly(allowedProduct); } @Test @@ -116,7 +116,7 @@ void discover_noCandidates_returnsEmptyResponse() { ProductDiscoveryResponseDTO result = productDiscoveryService.discover("client-1", "org-1", null, null, null); - assertThat(result.getProducts()).isEmpty(); + assertThat(result.products()).isEmpty(); } private PolicyInput argThatResource(String resource) { From 1bd80e2a5b8c3975fac25726be726187574a04db Mon Sep 17 00:00:00 2001 From: filipSzarek Date: Wed, 2 Sep 2026 13:48:35 +0200 Subject: [PATCH 10/15] test(product-discovery): resolve Sonar findings in ProductRepositoryTest - Use static imports for mock/withSettings/CALLS_REAL_METHODS instead of the Mockito.* qualified form (S8924). - Remove the useless eq(...) wraps in one verify() call where every argument used eq() and none needed a real matcher (S6068). --- .../persistency/repository/ProductRepositoryTest.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java index ad8d2cb..66b7f78 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/ProductRepositoryTest.java @@ -10,12 +10,14 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; import java.util.List; import org.junit.jupiter.api.Test; -import org.mockito.Mockito; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Product; @@ -30,7 +32,7 @@ class ProductRepositoryTest { private final ProductRepository productRepository = - Mockito.mock(ProductRepository.class, Mockito.withSettings().defaultAnswer(Mockito.CALLS_REAL_METHODS)); + mock(ProductRepository.class, withSettings().defaultAnswer(CALLS_REAL_METHODS)); @Test void findDiscoveryCandidates_escapesPercentAndUnderscoreInNameAndTopic() { @@ -40,8 +42,7 @@ void findDiscoveryCandidates_escapesPercentAndUnderscoreInNameAndTopic() { productRepository.findDiscoveryCandidates("Data_Feed", "topic%1", "TypeA", pageable); - verify(productRepository) - .findDiscoveryCandidatesByPattern(eq("%Data\\_Feed%"), eq("%topic\\%1%"), eq("TypeA"), eq(pageable)); + verify(productRepository).findDiscoveryCandidatesByPattern("%Data\\_Feed%", "%topic\\%1%", "TypeA", pageable); } @Test From d830d2bb9431e477944f19ce66649668d78219ad Mon Sep 17 00:00:00 2001 From: nikan Negaresh Date: Fri, 11 Sep 2026 11:55:19 +0100 Subject: [PATCH 11/15] feat(database): database structure and data change Add sample policy attributes for various scopes and update related configurations. - Added sample definitions, bindings, and values for ORGANISATION, CONSUMER, PRODUCER, PRODUCT, and SUBSCRIPTION policy scopes. - Adjusted entity mappings and test cases to align with updated `policy_*` table names. - Enhanced Vault container setup with support for unsealing in a development environment. --- .../KeycloakJwtAuthenticationConverter.java | 64 ++++++-- .../config/PolicyEnforcementInterceptor.java | 8 +- .../config/RequestRejectionSupport.java | 20 ++- .../v1/ProductDiscoveryController.java | 17 +- .../model/dto/PolicyAttributeDTO.java | 15 +- .../model/jwt/EnhancedPrincipal.java | 23 ++- .../node/management/model/jwt/JwtToken.java | 3 +- .../service/data/ProductDiscoveryService.java | 11 +- .../data/impl/PolicyAttributeServiceImpl.java | 4 +- .../impl/ProductDiscoveryServiceImpl.java | 17 +- .../service/providers/policy/PolicyInput.java | 22 ++- .../providers/policy/PolicyRequester.java | 27 ++++ ...20000__make_sample_grants_non_expiring.sql | 21 +++ .../CertificateValidationInterceptorTest.java | 2 +- .../config/ClientIdMdcFilterTest.java | 8 +- .../CustomJwtAuthenticationTokenTest.java | 4 +- ...thenticationConverterOrganisationTest.java | 149 ++++++++++++++++++ ...eycloakJwtAuthenticationConverterTest.java | 1 + .../PolicyEnforcementInterceptorTest.java | 10 +- .../v1/CertificateControllerSecurityTest.java | 3 +- .../v1/CertificateControllerTest.java | 3 +- ...ationPolicyEnforcementIntegrationTest.java | 2 +- .../v1/ProductDiscoveryControllerTest.java | 23 +-- ...olicyAttributeFieldsSerializationTest.java | 11 ++ .../management/model/jwt/JwtModelTest.java | 7 +- .../impl/PolicyAttributeServiceImplTest.java | 6 +- .../impl/ProductDiscoveryServiceImplTest.java | 19 ++- .../ConfigurationProviderImplTest.java | 16 +- ...ConfigPolicyAttributesIntegrationTest.java | 16 +- .../policy/PolicyDecisionClientTest.java | 3 +- .../PolicyDecisionSerializationTest.java | 22 ++- 31 files changed, 444 insertions(+), 113 deletions(-) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyRequester.java create mode 100644 src/main/resources/db/samples/V20260911120000__make_sample_grants_non_expiring.sql create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterOrganisationTest.java diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverter.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverter.java index ff1d69b..a577f09 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverter.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverter.java @@ -60,11 +60,13 @@ public class KeycloakJwtAuthenticationConverter implements Converter authorities = extractAuthorities(jwt); - EnhancedPrincipal principal = new EnhancedPrincipal(jwt.getSubject(), tokenClientId); + EnhancedPrincipal principal = + new EnhancedPrincipal(jwt.getSubject(), tokenClientId, extractOrganisation(jwt)); return new CustomJwtAuthenticationToken(jwt, authorities, principal); } catch (ResourceAccessParsingException e) { // If resource access parsing fails, log the error and fall back to JWT parsing @@ -171,7 +177,8 @@ public AbstractAuthenticationToken convert(Jwt jwt) { e.getMessage()); Collection authorities = extractAuthorities(jwt); - EnhancedPrincipal principal = new EnhancedPrincipal(jwt.getSubject(), tokenClientId); + EnhancedPrincipal principal = + new EnhancedPrincipal(jwt.getSubject(), tokenClientId, extractOrganisation(jwt)); return new CustomJwtAuthenticationToken(jwt, authorities, principal); } catch (Exception e) { // For any other unexpected exceptions @@ -179,7 +186,8 @@ public AbstractAuthenticationToken convert(Jwt jwt) { log.error("Unexpected error during JWT conversion for client ID: {}", tokenClientId, e); Collection authorities = extractAuthorities(jwt); - EnhancedPrincipal principal = new EnhancedPrincipal(jwt.getSubject(), tokenClientId); + EnhancedPrincipal principal = + new EnhancedPrincipal(jwt.getSubject(), tokenClientId, extractOrganisation(jwt)); return new CustomJwtAuthenticationToken(jwt, authorities, principal); } } @@ -192,15 +200,51 @@ public AbstractAuthenticationToken convert(Jwt jwt) { * @return A non-null client ID (either primary, fallback, or "unknown") */ private String getEffectiveClientId(String primaryId, String fallbackId) { - if (primaryId != null && !primaryId.isEmpty()) { - return primaryId; + return getEffectiveValue(primaryId, fallbackId, UNKNOWN_CLIENT); + } + + /** + * Returns the first of two candidate values that is neither null nor empty, or the + * supplied default when neither is usable. + * + * @param primary The preferred value + * @param fallback The value to use when primary is null or empty + * @param defaultValue The value to use when neither candidate is usable + * @return A non-null value + */ + private String getEffectiveValue(String primary, String fallback, String defaultValue) { + if (primary != null && !primary.isEmpty()) { + return primary; } - if (fallbackId != null && !fallbackId.isEmpty()) { - return fallbackId; + if (fallback != null && !fallback.isEmpty()) { + return fallback; } - return UNKNOWN_CLIENT; + return defaultValue; + } + + /** + * Extract the organisation from the JWT's "organisation" claim. + * Returns "unknown_organisation" when the claim is absent or empty, so the principal + * always carries a usable value. + */ + private String extractOrganisation(Jwt jwt) { + return getEffectiveValue(jwt.getClaimAsString(CLAIM_ORGANISATION), null, UNKNOWN_ORGANISATION); + } + + /** + * Extract the organisation from introspection data, falling back to the JWT's own + * "organisation" claim when introspection does not carry one - introspection is the more + * authoritative source, but an older authorisation server may not echo the claim back. + * + * @param jwtToken The data from the introspection endpoint + * @param jwt The JWT the introspection was performed for + * @return The organisation, or "unknown_organisation" when neither source has one + */ + private String extractOrganisationFromIntrospection(JwtToken jwtToken, Jwt jwt) { + return getEffectiveValue( + jwtToken.getOrganisation(), jwt.getClaimAsString(CLAIM_ORGANISATION), UNKNOWN_ORGANISATION); } /** diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptor.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptor.java index 7fdb7ce..eeac0fd 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptor.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptor.java @@ -17,6 +17,7 @@ import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecision; import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecisionClient; import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyInput; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyRequester; /** * Policy Enforcement Point: intercepts requests to policy-aware APIs, enriches them @@ -50,8 +51,11 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons String resource = request.getRequestURI(); String action = request.getMethod(); - String organisation = RequestRejectionSupport.getOrganisationId(request); - PolicyInput input = new PolicyInput(clientId, organisation, resource, action); + PolicyRequester requester = new PolicyRequester( + clientId, + RequestRejectionSupport.extractOrganisation(), + RequestRejectionSupport.getOrganisationId(request)); + PolicyInput input = PolicyInput.of(requester, resource, action); PolicyDecision decision = policyDecisionClient.evaluate(input); diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java index 761847d..98be42f 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/config/RequestRejectionSupport.java @@ -10,6 +10,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; +import java.util.function.Function; import org.springframework.http.MediaType; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; @@ -39,12 +40,27 @@ public static String getOrganisationId(HttpServletRequest request) { } static String extractClientId() { + return fromPrincipal(EnhancedPrincipal::clientId); + } + + /** + * The {@code organisation} claim carried on the authenticated principal. Distinct from + * {@link #getOrganisationId}, which is the organisation row id resolved from the client + * certificate - see {@code PolicyRequester}. + * + * @return the token's organisation, or null when there is no authenticated principal + */ + static String extractOrganisation() { + return fromPrincipal(EnhancedPrincipal::organisation); + } + + private static String fromPrincipal(Function accessor) { Authentication auth = SecurityContextHolder.getContext().getAuthentication(); if (auth == null || !(auth.getPrincipal() instanceof EnhancedPrincipal principal)) { return null; } - String clientId = principal.clientId(); - return (clientId == null || clientId.isEmpty()) ? null : clientId; + String value = accessor.apply(principal); + return (value == null || value.isEmpty()) ? null : value; } static void writeError( diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java index eb67dd3..be556d9 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryController.java @@ -27,6 +27,7 @@ import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductDiscoveryService; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyRequester; @RestController @RequestMapping("/api/v1/product") @@ -68,21 +69,19 @@ public ProductDiscoveryResponseDTO discoverProducts( ProductDiscoveryRequestDTO effectiveCriteria = criteria != null ? criteria : ProductDiscoveryRequestDTO.builder().build(); - String organisation = RequestRejectionSupport.getOrganisationId(request); + PolicyRequester requester = new PolicyRequester( + principal.clientId(), principal.organisation(), RequestRejectionSupport.getOrganisationId(request)); log.info( - "Product discovery request clientId={} organisation={} name={} topic={} type={}", - principal.clientId(), - organisation, + "Product discovery request clientId={} organisation={} organisationId={} name={} topic={} type={}", + requester.clientId(), + requester.organisation(), + requester.organisationId(), effectiveCriteria.name(), effectiveCriteria.topic(), effectiveCriteria.type()); return productDiscoveryService.discover( - principal.clientId(), - organisation, - effectiveCriteria.name(), - effectiveCriteria.topic(), - effectiveCriteria.type()); + requester, effectiveCriteria.name(), effectiveCriteria.topic(), effectiveCriteria.type()); } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeDTO.java index 20d93a1..8d8d83c 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeDTO.java @@ -14,9 +14,12 @@ /** * A policy attribute resolved from the {@code policy_attribute_scope}/{@code policy_attribute_definition}/ - * {@code policy_attribute_value} schema (added by PR #69) - the same three fields as {@link - * AttributesDTO} (the legacy {@code product_consumer_attribute}-backed representation), so it - * reads as a drop-in "policy" counterpart rather than a new shape to learn. + * {@code policy_attribute_value} schema. + * + *

{@code namespace} is carried as its own field rather than folded into {@code name} as a + * dotted prefix: consumers of this payload (policy rules, in particular) match on the namespace + * and the name separately, and splitting a dotted string back apart is both needless work and + * ambiguous once a name itself contains a dot. */ @Builder @Getter @@ -25,9 +28,11 @@ @AllArgsConstructor public class PolicyAttributeDTO { - /** The attribute's dotted {@code namespace.name} logical identifier (e.g. {@code "policy.risk-tier"}). */ + /** The namespace the attribute is defined in (e.g. {@code "policy"}). */ + private String namespace; + + /** The attribute's name within its namespace (e.g. {@code "risk-tier"}). */ private String name; private String value; - private String type; } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/EnhancedPrincipal.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/EnhancedPrincipal.java index 2eaf680..ac5c4d6 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/EnhancedPrincipal.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/EnhancedPrincipal.java @@ -1,6 +1,6 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ @@ -10,19 +10,26 @@ import java.io.Serializable; /** - * Custom Principal object that includes clientId information from the JWT. + * Custom Principal object that includes clientId and organisation information from the JWT. * - * @param subject -- GETTER -- - * Get the subject (user identifier) - * @param clientId -- GETTER -- - * Get the client ID + * @param subject -- GETTER -- + * Get the subject (user identifier) + * @param clientId -- GETTER -- + * Get the client ID + * @param organisation -- GETTER -- + * Get the organisation the token was issued for, taken from the + * {@code organisation} claim. Never null: falls back to + * {@code unknown_organisation} when the claim is absent, so callers + * do not have to null-check a value that is always present in the + * token shape this node expects. */ -public record EnhancedPrincipal(String subject, String clientId) implements Serializable { +public record EnhancedPrincipal(String subject, String clientId, String organisation) implements Serializable { @Serial private static final long serialVersionUID = 1L; @Override public String toString() { - return "CustomPrincipal{" + "subject='" + subject + '\'' + ", clientId='" + clientId + '\'' + '}'; + return "CustomPrincipal{" + "subject='" + subject + '\'' + ", clientId='" + clientId + '\'' + ", organisation='" + + organisation + '\'' + '}'; } } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtToken.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtToken.java index 30dcdbd..322150a 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtToken.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtToken.java @@ -1,6 +1,6 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ @@ -41,6 +41,7 @@ public class JwtToken { private Map resourceAccess; private String scope; + private String organisation; private String clientId; private String username; private String tokenType; diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java index c016a66..21d9467 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/ProductDiscoveryService.java @@ -9,6 +9,7 @@ import java.util.List; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDTO; import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyRequester; /** * Runs product discovery: queries candidate products matching the requester's search @@ -21,24 +22,22 @@ public interface ProductDiscoveryService { * Queries discovery candidates matching the given search criteria, then evaluates one * PDP decision per candidate, keeping only the ALLOWed ones. * - * @param clientId identity of the calling client - * @param organisation organisation the client belongs to, if known + * @param requester who is asking, as the PDP sees them * @param name optional case-insensitive contains filter on product name * @param topic optional case-insensitive contains filter on product topic * @param type optional case-insensitive exact filter on product type name * @return the products the requester is authorised to discover, matching the criteria */ - ProductDiscoveryResponseDTO discover(String clientId, String organisation, String name, String topic, String type); + ProductDiscoveryResponseDTO discover(PolicyRequester requester, String name, String topic, String type); /** * Evaluates one PDP decision per candidate product and returns only the ALLOWed ones. A * candidate is excluded (not the whole request failed) if the PDP denies it or the PDP * call itself fails, so a partial PDP outage degrades results rather than the request. * - * @param clientId identity of the calling client - * @param organisation organisation the client belongs to, if known + * @param requester who is asking, as the PDP sees them * @param candidates discovery candidate products to authorise * @return the subset of candidates the PDP allows for this requester */ - List filterAuthorised(String clientId, String organisation, List candidates); + List filterAuthorised(PolicyRequester requester, List candidates); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImpl.java index d4aa77d..a55ae9f 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImpl.java @@ -39,9 +39,9 @@ private PolicyAttributeDTO toDto(AttributeValue attributeValue) { AttributeDefinition definition = attributeValue.getAttributeDefinitionScope().getAttributeDefinition(); return PolicyAttributeDTO.builder() - .name(definition.getNamespace() + "." + definition.getName()) + .namespace(definition.getNamespace()) + .name(definition.getName()) .value(renderValue(attributeValue.getValue())) - .type(definition.getDataType()) .build(); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java index 549f4e6..e4d78d3 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImpl.java @@ -16,6 +16,7 @@ import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecision; import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecisionClient; import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyInput; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyRequester; /** * Reuses {@link PolicyDecisionClient} (built for the whole-request PEP on @@ -41,28 +42,26 @@ public ProductDiscoveryServiceImpl(ProductService productService, PolicyDecision } @Override - public ProductDiscoveryResponseDTO discover( - String clientId, String organisation, String name, String topic, String type) { + public ProductDiscoveryResponseDTO discover(PolicyRequester requester, String name, String topic, String type) { List candidates = productService.findDiscoveryCandidates(name, topic, type); - List authorised = filterAuthorised(clientId, organisation, candidates); + List authorised = filterAuthorised(requester, candidates); return ProductDiscoveryResponseDTO.builder().products(authorised).build(); } @Override - public List filterAuthorised(String clientId, String organisation, List candidates) { + public List filterAuthorised(PolicyRequester requester, List candidates) { return candidates.stream() - .filter(candidate -> isAuthorised(clientId, organisation, candidate)) + .filter(candidate -> isAuthorised(requester, candidate)) .toList(); } - private boolean isAuthorised(String clientId, String organisation, ProductDTO candidate) { - PolicyInput input = - new PolicyInput(clientId, organisation, PRODUCT_RESOURCE_PREFIX + candidate.getId(), DISCOVER_ACTION); + private boolean isAuthorised(PolicyRequester requester, ProductDTO candidate) { + PolicyInput input = PolicyInput.of(requester, PRODUCT_RESOURCE_PREFIX + candidate.getId(), DISCOVER_ACTION); PolicyDecision decision = policyDecisionClient.evaluate(input); if (decision == PolicyDecision.DENY) { log.debug( "Policy decision DENY clientId={} resource={} action={}", - clientId, + requester.clientId(), input.resource(), DISCOVER_ACTION); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java index ed22c89..27d60aa 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyInput.java @@ -16,10 +16,28 @@ * the request URI and HTTP method; per-candidate callers (e.g. product discovery) may use a * different convention, such as a stable resource id and a named action. * + *

{@code organisation} and {@code organisationId} are separate fields on purpose - see + * {@link PolicyRequester} for why - so a policy can read either, or require both to agree. + * * @param clientId identity of the calling client - * @param organisation organisation the client belongs to, if known + * @param organisation the token's {@code organisation} claim, if known + * @param organisationId organisation row id resolved from the client certificate, if known * @param resource the resource being evaluated, in whatever convention the caller uses * @param action the action being evaluated, in whatever convention the caller uses */ @JsonInclude(JsonInclude.Include.NON_NULL) -public record PolicyInput(String clientId, String organisation, String resource, String action) {} +public record PolicyInput(String clientId, String organisation, String organisationId, String resource, String action) { + + /** + * Builds an input for one decision about {@code resource}/{@code action} by {@code requester}. + * + * @param requester who is asking + * @param resource the resource being evaluated + * @param action the action being evaluated + * @return the PDP input + */ + public static PolicyInput of(PolicyRequester requester, String resource, String action) { + return new PolicyInput( + requester.clientId(), requester.organisation(), requester.organisationId(), resource, action); + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyRequester.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyRequester.java new file mode 100644 index 0000000..1b19b6b --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyRequester.java @@ -0,0 +1,27 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.providers.policy; + +/** + * Who is making a request, as far as the PDP is concerned. Groups the three identity values + * so they travel together rather than as interchangeable {@code String} parameters, and so a + * caller cannot silently transpose the two organisation values. + * + *

The two organisation fields are deliberately distinct and independently sourced: + * {@code organisation} comes from the access token, {@code organisationId} from the client + * certificate. They answer different questions ("which organisation does the IdP say issued + * this token" versus "which organisation row does this certificate belong to"), and a policy + * may legitimately require both, or cross-check one against the other. + * + * @param clientId identity of the calling client, from the token's {@code azp}/{@code client_id} + * @param organisation the token's {@code organisation} claim (e.g. {@code FEDERATOR_ENV}), or + * {@code unknown_organisation} when the token carries no such claim + * @param organisationId id of the {@code organisation} row resolved from the client certificate + * by {@link uk.gov.dbt.ndtp.ia.node.management.config.CertificateValidationInterceptor}, or + * {@code null} on a request that did not go through certificate validation + */ +public record PolicyRequester(String clientId, String organisation, String organisationId) {} diff --git a/src/main/resources/db/samples/V20260911120000__make_sample_grants_non_expiring.sql b/src/main/resources/db/samples/V20260911120000__make_sample_grants_non_expiring.sql new file mode 100644 index 0000000..213f9fe --- /dev/null +++ b/src/main/resources/db/samples/V20260911120000__make_sample_grants_non_expiring.sql @@ -0,0 +1,21 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +-- The sample grants in V20250728152300 were written with a fixed granted_ts of 2025-07-01 and a +-- validity of 365 days, so they silently expired on 2026-07-01: ConfigurationProviderImpl drops any +-- product_consumer row where granted_ts + validity days is in the past, which empties both +-- `consumers` and `configurations` on GET /api/v1/configuration/producer. +-- +-- validity = 0 means "no expiry" to isValidProvider, so grants stay usable indefinitely rather than +-- rotting a year after whoever wrote the fixed date. +-- +-- This applies to every product_consumer row, not just the pairings seeded by V20250728152300 - so a +-- grant added by hand while working locally does not expire either. It lives in db/samples, which is +-- on the Flyway path for local and dev profiles only (see spring.flyway.locations); it must not be +-- promoted to db/migration, where it would clear the expiry on real grants. +UPDATE product_consumer +SET validity = 0 +WHERE validity IS DISTINCT FROM 0; diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CertificateValidationInterceptorTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CertificateValidationInterceptorTest.java index ed20d67..567c7c0 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CertificateValidationInterceptorTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CertificateValidationInterceptorTest.java @@ -76,7 +76,7 @@ void tearDown() throws Exception { } private void setupAuthentication(String clientId) { - EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId, "test-organisation"); when(securityContext.getAuthentication()).thenReturn(authentication); when(authentication.getPrincipal()).thenReturn(principal); } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/ClientIdMdcFilterTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/ClientIdMdcFilterTest.java index 62f4663..91568a5 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/ClientIdMdcFilterTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/ClientIdMdcFilterTest.java @@ -65,7 +65,7 @@ void tearDown() throws Exception { @Test void doFilterInternal_withEnhancedPrincipal_shouldSetMdc() throws ServletException, IOException { // Arrange - EnhancedPrincipal principal = new EnhancedPrincipal("subject", "test-client-id"); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", "test-client-id", "test-organisation"); when(securityContext.getAuthentication()).thenReturn(authentication); when(authentication.getPrincipal()).thenReturn(principal); when(authentication.getName()).thenReturn("test-user"); @@ -109,7 +109,7 @@ void doFilterInternal_withNonEnhancedPrincipal_shouldSetEmptyMdc() throws Servle void doFilterInternal_withEnhancedPrincipalButEmptyClientId_shouldSetEmptyMdc() throws ServletException, IOException { // Arrange - EnhancedPrincipal principal = new EnhancedPrincipal("subject", ""); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", "", "test-organisation"); when(securityContext.getAuthentication()).thenReturn(authentication); when(authentication.getPrincipal()).thenReturn(principal); @@ -125,7 +125,7 @@ void doFilterInternal_withEnhancedPrincipalButEmptyClientId_shouldSetEmptyMdc() void doFilterInternal_withEnhancedPrincipalButNullClientId_shouldSetEmptyMdc() throws ServletException, IOException { // Arrange - EnhancedPrincipal principal = new EnhancedPrincipal("subject", null); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", null, "test-organisation"); when(securityContext.getAuthentication()).thenReturn(authentication); when(authentication.getPrincipal()).thenReturn(principal); @@ -154,7 +154,7 @@ void doFilterInternal_withNullPrincipal_shouldSetEmptyMdc() throws ServletExcept @Test void doFilterInternal_shouldClearMdcEvenOnException() throws ServletException, IOException { // Arrange - EnhancedPrincipal principal = new EnhancedPrincipal("subject", "test-client-id"); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", "test-client-id", "test-organisation"); when(securityContext.getAuthentication()).thenReturn(authentication); when(authentication.getPrincipal()).thenReturn(principal); doThrow(new RuntimeException("Test exception")).when(filterChain).doFilter(request, response); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CustomJwtAuthenticationTokenTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CustomJwtAuthenticationTokenTest.java index 1f99147..4327cf0 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CustomJwtAuthenticationTokenTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/CustomJwtAuthenticationTokenTest.java @@ -32,7 +32,7 @@ void setUp() { Map claims = new HashMap<>(); claims.put("sub", "test-subject"); jwt = new Jwt("token-value", Instant.now(), Instant.now().plusSeconds(3600), headers, claims); - principal = new EnhancedPrincipal("test-subject", "test-client"); + principal = new EnhancedPrincipal("test-subject", "test-client", "test-organisation"); authorities = Collections.singletonList(new SimpleGrantedAuthority("ROLE_USER")); } @@ -49,7 +49,7 @@ void equalsAndHashCode() { CustomJwtAuthenticationToken token1 = new CustomJwtAuthenticationToken(jwt, authorities, principal); CustomJwtAuthenticationToken token2 = new CustomJwtAuthenticationToken(jwt, authorities, principal); - EnhancedPrincipal principal2 = new EnhancedPrincipal("other-subject", "test-client"); + EnhancedPrincipal principal2 = new EnhancedPrincipal("other-subject", "test-client", "test-organisation"); CustomJwtAuthenticationToken token3 = new CustomJwtAuthenticationToken(jwt, authorities, principal2); // Equals diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterOrganisationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterOrganisationTest.java new file mode 100644 index 0000000..cd8607e --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterOrganisationTest.java @@ -0,0 +1,149 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.config; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; + +import java.time.Instant; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.security.authentication.AbstractAuthenticationToken; +import org.springframework.security.oauth2.jwt.Jwt; +import org.springframework.test.util.ReflectionTestUtils; +import org.springframework.web.client.RestClientException; +import org.springframework.web.client.RestTemplate; +import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; +import uk.gov.dbt.ndtp.ia.node.management.model.jwt.JwtToken; + +/** + * Covers the {@code organisation} claim reaching {@link EnhancedPrincipal}, from both the + * introspection response and the JWT itself. Claim values are those of a real FEDERATOR_ENV + * token, where {@code organisation} and {@code azp} happen to agree - the assertions here + * deliberately do not rely on that, so a token whose organisation differs from its client id + * would still be read correctly. + */ +@ExtendWith(MockitoExtension.class) +class KeycloakJwtAuthenticationConverterOrganisationTest { + + private static final String ORGANISATION = "FEDERATOR_ENV"; + private static final String UNKNOWN_ORGANISATION = "unknown_organisation"; + private static final String SUBJECT = "d17e51cf-ef3c-4adf-b51a-34a5f8b6c4f7"; + + @Mock + private RestTemplate restTemplate; + + @InjectMocks + private KeycloakJwtAuthenticationConverter converter; + + @BeforeEach + void setUp() { + ReflectionTestUtils.setField( + converter, + "introspectionUri", + "https://localhost:8443/realms/mng-node/protocol/openid-connect/token/introspect"); + ReflectionTestUtils.setField(converter, "restTemplate", restTemplate); + } + + private Jwt jwtWith(String organisation) { + Map headers = Map.of("alg", "RS256", "typ", "JWT"); + + Map claims = new HashMap<>(); + claims.put("iss", "https://localhost:8443/realms/mng-node"); + claims.put("aud", "management-node"); + claims.put("sub", SUBJECT); + claims.put("typ", "Bearer"); + claims.put("azp", "FEDERATOR_ENV"); + claims.put("scope", "FEDERATOR_PRODUCER MANAGEMENT_NODE_ACCESS FEDERATOR_CONSUMER"); + claims.put( + "resource_access", + Map.of("management-node", Map.of("roles", List.of("access_producer_configurations", "create_keys")))); + if (organisation != null) { + claims.put("organisation", organisation); + } + + return new Jwt( + "token-value", Instant.ofEpochSecond(1789118956), Instant.ofEpochSecond(1789120757), headers, claims); + } + + private void stubIntrospection(String organisation) { + JwtToken introspection = JwtToken.builder() + .active(true) + .sub(SUBJECT) + .azp("FEDERATOR_ENV") + .organisation(organisation) + .resourceAccess(Map.of( + "management-node", + JwtToken.ResourceAccess.builder() + .roles(List.of("access_producer_configurations")) + .build())) + .build(); + + when(restTemplate.postForEntity(anyString(), any(HttpEntity.class), org.mockito.Mockito.eq(JwtToken.class))) + .thenReturn(new ResponseEntity<>(introspection, HttpStatus.OK)); + } + + private String organisationOf(AbstractAuthenticationToken token) { + return ((EnhancedPrincipal) token.getPrincipal()).organisation(); + } + + @Test + void convert_shouldTakeOrganisationFromIntrospection() { + stubIntrospection(ORGANISATION); + + assertEquals(ORGANISATION, organisationOf(converter.convert(jwtWith(ORGANISATION)))); + } + + @Test + void convert_shouldFallBackToJwtClaimWhenIntrospectionHasNoOrganisation() { + stubIntrospection(null); + + assertEquals(ORGANISATION, organisationOf(converter.convert(jwtWith(ORGANISATION)))); + } + + @Test + void convert_shouldUseUnknownOrganisationWhenNeitherSourceHasOne() { + stubIntrospection(null); + + assertEquals(UNKNOWN_ORGANISATION, organisationOf(converter.convert(jwtWith(null)))); + } + + @Test + void convert_shouldReadOrganisationFromJwtWhenIntrospectionFails() { + when(restTemplate.postForEntity(anyString(), any(HttpEntity.class), org.mockito.Mockito.eq(JwtToken.class))) + .thenThrow(new RestClientException("introspection endpoint unavailable")); + + assertEquals(ORGANISATION, organisationOf(converter.convert(jwtWith(ORGANISATION)))); + } + + @Test + void convert_shouldUseUnknownOrganisationWhenIntrospectionFailsAndClaimIsAbsent() { + when(restTemplate.postForEntity(anyString(), any(HttpEntity.class), org.mockito.Mockito.eq(JwtToken.class))) + .thenThrow(new RestClientException("introspection endpoint unavailable")); + + assertEquals(UNKNOWN_ORGANISATION, organisationOf(converter.convert(jwtWith(null)))); + } + + @Test + void convert_shouldTreatEmptyOrganisationAsUnknown() { + stubIntrospection(""); + + assertEquals(UNKNOWN_ORGANISATION, organisationOf(converter.convert(jwtWith("")))); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterTest.java index 8f50bed..b44b82d 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/KeycloakJwtAuthenticationConverterTest.java @@ -185,6 +185,7 @@ private JwtToken createJwtTokenFromMap(Map map) { .allowedOrigins((List) map.get("allowed-origins")) .resourceAccess(resourceAccess) .scope((String) map.get("scope")) + .organisation((String) map.get("organisation")) .username((String) map.get("username")) .tokenType((String) map.get("token_type")) .build(); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptorTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptorTest.java index e57aa07..b8e95c3 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptorTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/config/PolicyEnforcementInterceptorTest.java @@ -80,7 +80,7 @@ void tearDown() throws Exception { } private void setupAuthentication(String clientId) { - EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId, "test-organisation"); when(securityContext.getAuthentication()).thenReturn(authentication); when(authentication.getPrincipal()).thenReturn(principal); } @@ -151,11 +151,12 @@ void policyInput_includesClientResourceAndAction() throws Exception { interceptor.preHandle(request, response, handlerMethod); verify(policyDecisionClient) - .evaluate(new PolicyInput("client-1", null, "/api/v1/configuration/consumer", "GET")); + .evaluate(new PolicyInput( + "client-1", "test-organisation", null, "/api/v1/configuration/consumer", "GET")); } @Test - void policyInput_includesOrganisationResolvedByCertificateValidation() throws Exception { + void policyInput_includesBothTokenOrganisationAndCertificateOrganisationId() throws Exception { setupAuthentication("client-1"); when(request.getRequestURI()).thenReturn("/api/v1/configuration/consumer"); when(request.getMethod()).thenReturn("GET"); @@ -165,7 +166,8 @@ void policyInput_includesOrganisationResolvedByCertificateValidation() throws Ex interceptor.preHandle(request, response, handlerMethod); verify(policyDecisionClient) - .evaluate(new PolicyInput("client-1", "42", "/api/v1/configuration/consumer", "GET")); + .evaluate(new PolicyInput( + "client-1", "test-organisation", "42", "/api/v1/configuration/consumer", "GET")); } @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerSecurityTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerSecurityTest.java index 3a0b08f..cea44fd 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerSecurityTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerSecurityTest.java @@ -64,7 +64,8 @@ CertificateController certificateController( @Autowired private CertificateController controller; - private static final EnhancedPrincipal TEST_PRINCIPAL = new EnhancedPrincipal("sub", "client-1"); + private static final EnhancedPrincipal TEST_PRINCIPAL = + new EnhancedPrincipal("sub", "client-1", "test-organisation"); private static final SignCertRequestDTO SIGN_REQUEST = SignCertRequestDTO.builder().csr("CSR").build(); private static final CreateCsrRequestDTO CSR_REQUEST = new CreateCsrRequestDTO(); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerTest.java index 1d03468..4b1a95e 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/CertificateControllerTest.java @@ -52,7 +52,8 @@ class CertificateControllerTest { @InjectMocks private CertificateController certificateController; - private static final EnhancedPrincipal TEST_PRINCIPAL = new EnhancedPrincipal("subject", "client-1"); + private static final EnhancedPrincipal TEST_PRINCIPAL = + new EnhancedPrincipal("subject", "client-1", "test-organisation"); @BeforeEach void setUp() { diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java index 3ef2a28..7ac070b 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ConfigurationPolicyEnforcementIntegrationTest.java @@ -65,7 +65,7 @@ void tearDown() { } private void authenticateAs(String clientId) { - EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId, "test-organisation"); Authentication authentication = mock(Authentication.class); when(authentication.getPrincipal()).thenReturn(principal); SecurityContext context = mock(SecurityContext.class); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java index 40ac4d6..8cc6405 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java @@ -7,7 +7,6 @@ package uk.gov.dbt.ndtp.ia.node.management.controller.v1; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.lenient; @@ -38,6 +37,7 @@ import uk.gov.dbt.ndtp.ia.node.management.model.dto.ProductDiscoveryResponseDTO; import uk.gov.dbt.ndtp.ia.node.management.model.jwt.EnhancedPrincipal; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductDiscoveryService; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyRequester; /** * Integration test for {@code POST /api/v1/product/discovery} wiring @@ -71,7 +71,7 @@ void tearDown() { private void authenticateAs(String clientId) { // lenient: not every test (e.g. request-validation-failure tests) reaches argument // resolution far enough to consult these mocks - EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId); + EnhancedPrincipal principal = new EnhancedPrincipal("subject", clientId, "test-organisation"); Authentication authentication = mock(Authentication.class); lenient().when(authentication.getPrincipal()).thenReturn(principal); SecurityContext context = mock(SecurityContext.class); @@ -86,7 +86,7 @@ private static ProductDiscoveryResponseDTO responseWith(ProductDTO... products) @Test void fullyPermitted_returnsAllCandidates() throws Exception { ProductDTO product = ProductDTO.builder().id(1L).name("Alpha").build(); - when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + when(productDiscoveryService.discover(any(PolicyRequester.class), any(), any(), any())) .thenReturn(responseWith(product)); mockMvc.perform(post("/api/v1/product/discovery") @@ -100,7 +100,7 @@ void fullyPermitted_returnsAllCandidates() throws Exception { @Test void partiallyFiltered_returnsOnlyAuthorisedSubset() throws Exception { ProductDTO allowed = ProductDTO.builder().id(1L).name("Allowed").build(); - when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + when(productDiscoveryService.discover(any(PolicyRequester.class), any(), any(), any())) .thenReturn(responseWith(allowed)); mockMvc.perform(post("/api/v1/product/discovery") @@ -113,7 +113,7 @@ void partiallyFiltered_returnsOnlyAuthorisedSubset() throws Exception { @Test void noCandidates_returnsEmptyListNotError() throws Exception { - when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + when(productDiscoveryService.discover(any(PolicyRequester.class), any(), any(), any())) .thenReturn(responseWith()); mockMvc.perform(post("/api/v1/product/discovery") @@ -125,7 +125,7 @@ void noCandidates_returnsEmptyListNotError() throws Exception { @Test void noAuthorisedProducts_returnsEmptyListNotErrorAndPassesCriteriaThrough() throws Exception { - when(productDiscoveryService.discover(anyString(), any(), any(), any(), any())) + when(productDiscoveryService.discover(any(PolicyRequester.class), any(), any(), any())) .thenReturn(responseWith()); mockMvc.perform(post("/api/v1/product/discovery") @@ -134,7 +134,12 @@ void noAuthorisedProducts_returnsEmptyListNotErrorAndPassesCriteriaThrough() thr .andExpect(status().isOk()) .andExpect(jsonPath("$.products").isEmpty()); - verify(productDiscoveryService).discover(eq("client-1"), any(), eq("Alpha"), eq("topic-1"), eq("TypeA")); + verify(productDiscoveryService) + .discover( + eq(new PolicyRequester("client-1", "test-organisation", null)), + eq("Alpha"), + eq("topic-1"), + eq("TypeA")); } @Test @@ -142,7 +147,7 @@ void filterMatchingDeniedProduct_stillExcludedFromResponse() throws Exception { // A search filter matching a product does not widen what the PDP authorises: the // candidate query narrows by filter, but the PDP filter (mocked here as denying it) // still wins. - when(productDiscoveryService.discover(anyString(), any(), eq("Restricted"), any(), any())) + when(productDiscoveryService.discover(any(PolicyRequester.class), eq("Restricted"), any(), any())) .thenReturn(responseWith()); mockMvc.perform(post("/api/v1/product/discovery") @@ -176,7 +181,7 @@ void malformedJsonBody_returns400() throws Exception { @Test void emptyBody_treatedAsNoFilter() throws Exception { - when(productDiscoveryService.discover(anyString(), any(), isNull(), isNull(), isNull())) + when(productDiscoveryService.discover(any(PolicyRequester.class), isNull(), isNull(), isNull())) .thenReturn(responseWith()); mockMvc.perform(post("/api/v1/product/discovery").contentType(MediaType.APPLICATION_JSON)) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java index ba54518..764c49a 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java @@ -21,6 +21,17 @@ class PolicyAttributeFieldsSerializationTest { private final ObjectMapper objectMapper = new ObjectMapper(); + @Test + void policyAttribute_serialisesNamespaceNameAndValueOnly() throws Exception { + String json = objectMapper.writeValueAsString(PolicyAttributeDTO.builder() + .namespace("policy") + .name("risk-tier") + .value("gold") + .build()); + + assertThat(json).isEqualTo("{\"namespace\":\"policy\",\"name\":\"risk-tier\",\"value\":\"gold\"}"); + } + @Test void producerDto_policyAttributesSerialisesAsEmptyArray() throws Exception { JsonNode json = objectMapper.readTree( diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtModelTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtModelTest.java index d96d22a..ce45b7f 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtModelTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/jwt/JwtModelTest.java @@ -18,12 +18,14 @@ class JwtModelTest { @Test void testEnhancedPrincipal() { - EnhancedPrincipal principal = new EnhancedPrincipal("user123", "client456"); + EnhancedPrincipal principal = new EnhancedPrincipal("user123", "client456", "test-organisation"); assertEquals("user123", principal.subject()); assertEquals("client456", principal.clientId()); + assertEquals("test-organisation", principal.organisation()); String toString = principal.toString(); assertTrue(toString.contains("user123")); assertTrue(toString.contains("client456")); + assertTrue(toString.contains("test-organisation")); } @Test @@ -31,6 +33,7 @@ void testJwtToken() { JwtToken token = JwtToken.builder() .sub("subject") .clientId("client") + .organisation("FEDERATOR_ENV") .active(true) .aud(List.of("aud1")) .resourceAccess(Map.of( @@ -42,6 +45,7 @@ void testJwtToken() { assertEquals("subject", token.getSub()); assertEquals("client", token.getClientId()); + assertEquals("FEDERATOR_ENV", token.getOrganisation()); assertTrue(token.getActive()); assertEquals(List.of("aud1"), token.getAud()); assertNotNull(token.getResourceAccess()); @@ -52,6 +56,7 @@ void testJwtToken() { JwtToken token2 = JwtToken.builder() .sub("subject") .clientId("client") + .organisation("FEDERATOR_ENV") .active(true) .aud(List.of("aud1")) .resourceAccess(Map.of( diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImplTest.java index 51ef86d..ec15bf1 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/PolicyAttributeServiceImplTest.java @@ -52,7 +52,7 @@ private static AttributeValue attributeValue(String namespace, String name, Stri } @Test - void findAttributes_mapsNamespaceDotNameValueAndType() { + void findAttributes_mapsNamespaceNameAndValueAsSeparateFields() { when(attributeValueRepository.findLiveByEntityIdAndScopeCode(10L, "PRODUCER")) .thenReturn(List.of(attributeValue("policy", "risk-tier", "STRING", "\"gold\""))); @@ -60,9 +60,9 @@ void findAttributes_mapsNamespaceDotNameValueAndType() { assertThat(result).hasSize(1); PolicyAttributeDTO dto = result.get(0); - assertThat(dto.getName()).isEqualTo("policy.risk-tier"); + assertThat(dto.getNamespace()).isEqualTo("policy"); + assertThat(dto.getName()).isEqualTo("risk-tier"); assertThat(dto.getValue()).isEqualTo("gold"); - assertThat(dto.getType()).isEqualTo("STRING"); } @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java index c0f3615..a4495a3 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/ProductDiscoveryServiceImplTest.java @@ -24,6 +24,7 @@ import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecision; import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyDecisionClient; import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyInput; +import uk.gov.dbt.ndtp.ia.node.management.service.providers.policy.PolicyRequester; /** * Verifies {@link ProductDiscoveryServiceImpl} queries candidates then evaluates one PDP @@ -34,6 +35,8 @@ @ExtendWith(MockitoExtension.class) class ProductDiscoveryServiceImplTest { + private static final PolicyRequester REQUESTER = new PolicyRequester("client-1", "FEDERATOR_ENV", "org-1"); + @Mock private ProductService productService; @@ -57,7 +60,7 @@ void filterAuthorised_fullyPermitted_returnsAllCandidates() { when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.ALLOW); List result = - productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct, deniedProduct)); + productDiscoveryService.filterAuthorised(REQUESTER, List.of(allowedProduct, deniedProduct)); assertThat(result).containsExactlyInAnyOrder(allowedProduct, deniedProduct); } @@ -71,7 +74,7 @@ void filterAuthorised_partiallyPermitted_returnsOnlyAllowedAndLeaksNoDeniedData( when(policyDecisionClient.evaluate(argThatResource("product:2"))).thenReturn(PolicyDecision.DENY); List result = - productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct, deniedProduct)); + productDiscoveryService.filterAuthorised(REQUESTER, List.of(allowedProduct, deniedProduct)); assertThat(result).containsExactly(allowedProduct); assertThat(result).extracting(ProductDTO::getId).doesNotContain(2L); @@ -83,7 +86,7 @@ void filterAuthorised_noneAuthorised_returnsEmptyList() { when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.DENY); List result = - productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct, deniedProduct)); + productDiscoveryService.filterAuthorised(REQUESTER, List.of(allowedProduct, deniedProduct)); assertThat(result).isEmpty(); } @@ -92,9 +95,10 @@ void filterAuthorised_noneAuthorised_returnsEmptyList() { void filterAuthorised_buildsPolicyInputWithDiscoverActionAndProductResource() { when(policyDecisionClient.evaluate(any())).thenReturn(PolicyDecision.ALLOW); - productDiscoveryService.filterAuthorised("client-1", "org-1", List.of(allowedProduct)); + productDiscoveryService.filterAuthorised(REQUESTER, List.of(allowedProduct)); - verify(policyDecisionClient).evaluate(new PolicyInput("client-1", "org-1", "product:1", "discover")); + verify(policyDecisionClient) + .evaluate(new PolicyInput("client-1", "FEDERATOR_ENV", "org-1", "product:1", "discover")); } @Test @@ -104,8 +108,7 @@ void discover_queriesCandidatesThenFiltersByPolicy() { when(policyDecisionClient.evaluate(argThatResource("product:1"))).thenReturn(PolicyDecision.ALLOW); when(policyDecisionClient.evaluate(argThatResource("product:2"))).thenReturn(PolicyDecision.DENY); - ProductDiscoveryResponseDTO result = - productDiscoveryService.discover("client-1", "org-1", "Alpha", "topic-1", "TypeA"); + ProductDiscoveryResponseDTO result = productDiscoveryService.discover(REQUESTER, "Alpha", "topic-1", "TypeA"); assertThat(result.products()).containsExactly(allowedProduct); } @@ -114,7 +117,7 @@ void discover_queriesCandidatesThenFiltersByPolicy() { void discover_noCandidates_returnsEmptyResponse() { when(productService.findDiscoveryCandidates(any(), any(), any())).thenReturn(List.of()); - ProductDiscoveryResponseDTO result = productDiscoveryService.discover("client-1", "org-1", null, null, null); + ProductDiscoveryResponseDTO result = productDiscoveryService.discover(REQUESTER, null, null, null); assertThat(result.products()).isEmpty(); } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index d3d62c1..1b63cf3 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -394,24 +394,24 @@ void getProducerConfigByClientId_populatesPolicyAttributesForEveryScope() { when(consumerService.findById(701L)).thenReturn(Optional.of(consumer)); PolicyAttributeDTO producerAttr = PolicyAttributeDTO.builder() - .name("policy.a") + .namespace("policy") + .name("a") .value("1") - .type("STRING") .build(); PolicyAttributeDTO consumerAttr = PolicyAttributeDTO.builder() - .name("policy.b") + .namespace("policy") + .name("b") .value("2") - .type("STRING") .build(); PolicyAttributeDTO orgAttr = PolicyAttributeDTO.builder() - .name("policy.c") + .namespace("policy") + .name("c") .value("3") - .type("STRING") .build(); PolicyAttributeDTO subscriptionAttr = PolicyAttributeDTO.builder() - .name("policy.d") + .namespace("policy") + .name("d") .value("4") - .type("STRING") .build(); when(policyAttributeService.findAttributes(70L, PolicyAttributeScope.PRODUCER)) .thenReturn(List.of(producerAttr)); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java index cce6505..45a84df 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java @@ -234,22 +234,22 @@ void producerConfig_carriesPolicyAttributesAcrossAllFourScopes_andEmptyForSiblin ProducerDTO producerDto = cfg.getProducers().get(0); assertThat(producerDto.getPolicyAttributes()) - .extracting(PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue, PolicyAttributeDTO::getType) - .containsExactly(tuple("policy.producer-tier", "gold", "STRING")); + .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) + .containsExactly(tuple("policy", "producer-tier", "gold")); ProductDTO productDto = producerDto.getProducts().get(0); ConsumerDTO consumerDto = productDto.getConsumers().get(0); assertThat(consumerDto.getPolicyAttributes()) - .extracting(PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue, PolicyAttributeDTO::getType) - .containsExactly(tuple("policy.consumer-tier", "silver", "STRING")); + .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) + .containsExactly(tuple("policy", "consumer-tier", "silver")); assertThat(consumerDto.getOrganisationPolicyAttributes()) - .extracting(PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue, PolicyAttributeDTO::getType) - .containsExactly(tuple("policy.org-region", "uk", "STRING")); + .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) + .containsExactly(tuple("policy", "org-region", "uk")); ProductConsumerDTO subscriptionDto = productDto.getConfigurations().get(0); assertThat(subscriptionDto.getPolicyAttributes()) - .extracting(PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue, PolicyAttributeDTO::getType) - .containsExactly(tuple("policy.sub-priority", "1", "STRING")); + .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) + .containsExactly(tuple("policy", "sub-priority", "1")); ProducerConfigDTO bareCfg = configurationProvider().getProducerConfigByClientId("client-no-attrs", Optional.empty()); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionClientTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionClientTest.java index f16e23f..0ebf063 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionClientTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionClientTest.java @@ -26,7 +26,8 @@ class PolicyDecisionClientTest { - private static final PolicyInput INPUT = new PolicyInput("client-1", null, "/api/v1/configuration/producer", "GET"); + private static final PolicyInput INPUT = + new PolicyInput("client-1", null, null, "/api/v1/configuration/producer", "GET"); private static final OpaProperties PROPERTIES = new OpaProperties( "https://opa.example.internal", "/v1/data/management_node/allow", diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionSerializationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionSerializationTest.java index 87e53ae..56b8bbf 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionSerializationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/policy/PolicyDecisionSerializationTest.java @@ -18,28 +18,40 @@ class PolicyDecisionSerializationTest { @Test void request_serializesWithAllAttributes() throws Exception { PolicyDecisionRequest request = new PolicyDecisionRequest( - new PolicyInput("client-1", "org-1", "/api/v1/configuration/producer", "GET")); + new PolicyInput("client-1", "FEDERATOR_ENV", "42", "/api/v1/configuration/producer", "GET")); String json = objectMapper.writeValueAsString(request); assertThat(json) .isEqualTo( - "{\"input\":{\"clientId\":\"client-1\",\"organisation\":\"org-1\",\"resource\":\"/api/v1/configuration/producer\",\"action\":\"GET\"}}"); + "{\"input\":{\"clientId\":\"client-1\",\"organisation\":\"FEDERATOR_ENV\",\"organisationId\":\"42\"," + + "\"resource\":\"/api/v1/configuration/producer\",\"action\":\"GET\"}}"); } @Test - void request_omitsOrganisationWhenNull() throws Exception { - PolicyDecisionRequest request = - new PolicyDecisionRequest(new PolicyInput("client-1", null, "/api/v1/configuration/producer", "GET")); + void request_omitsOrganisationFieldsWhenNull() throws Exception { + PolicyDecisionRequest request = new PolicyDecisionRequest( + new PolicyInput("client-1", null, null, "/api/v1/configuration/producer", "GET")); String json = objectMapper.writeValueAsString(request); assertThat(json) .doesNotContain("organisation") + .doesNotContain("organisationId") .isEqualTo( "{\"input\":{\"clientId\":\"client-1\",\"resource\":\"/api/v1/configuration/producer\",\"action\":\"GET\"}}"); } + @Test + void request_keepsTokenOrganisationWhenCertificateIdAbsent() throws Exception { + PolicyDecisionRequest request = new PolicyDecisionRequest( + new PolicyInput("client-1", "FEDERATOR_ENV", null, "/api/v1/configuration/producer", "GET")); + + String json = objectMapper.writeValueAsString(request); + + assertThat(json).contains("\"organisation\":\"FEDERATOR_ENV\"").doesNotContain("organisationId"); + } + @Test void response_deserializesAllowResult() throws Exception { PolicyDecisionResponse response = objectMapper.readValue("{\"result\":true}", PolicyDecisionResponse.class); From a5b35195b3b03c0711894c66c5cd865631a7d3e7 Mon Sep 17 00:00:00 2001 From: nikan Negaresh Date: Fri, 11 Sep 2026 16:40:42 +0100 Subject: [PATCH 12/15] feat(config): introduce organisation key and associated functionality - Added a stable, human-readable `organisation_key` column to the `organisation` table with unique constraint and migration SQL. - Backfilled keys for existing records using name-based transformations. - Enhanced `Organisation` entity to include `organisationKey` field. - Introduced `OrganisationDTO`, `OrganisationConverter`, and `OrganisationServiceImpl` for exposing organisation data, including the new key. - Updated repositories with new methods for querying by `organisationKey` and checking key existence. - Adjusted DTOs, tests, and configurations to support the new `organisationKey` functionality and related policy attributes. - Updated integration tests and backfilled data validations. --- docs/DATABASE_SCHEMA.md | 6 + .../converter/impl/OrganisationConverter.java | 46 ++++ .../management/model/dto/ConsumerDTO.java | 8 +- .../management/model/dto/OrganisationDTO.java | 38 +++ .../model/dto/ProducerConfigDTO.java | 10 +- .../management/model/dto/ProducerDTO.java | 3 + .../node/management/model/dto/ProductDTO.java | 11 +- .../persistency/entity/Organisation.java | 9 +- .../repository/OrganisationRepository.java | 23 +- .../service/data/OrganisationService.java | 35 ++- .../service/data/PolicyAttributeScope.java | 12 +- .../data/impl/OrganisationServiceImpl.java | 65 ++++++ .../ConfigurationProviderImpl.java | 143 +++++++++++- .../V20260911130000__add_organisation_key.sql | 35 +++ .../impl/OrganisationConverterTest.java | 69 ++++++ ...olicyAttributeFieldsSerializationTest.java | 62 ++++- .../AttributeValueSoftDeleteTriggerTest.java | 4 + .../OrganisationKeyRepositoryTest.java | 87 +++++++ .../data/PolicyAttributeScopeTest.java | 9 +- .../impl/OrganisationServiceImplTest.java | 135 +++++++++++ .../ConfigurationProviderImplTest.java | 219 +++++++++++++++++- ...ConfigPolicyAttributesIntegrationTest.java | 31 ++- 22 files changed, 1035 insertions(+), 25 deletions(-) create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverter.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/OrganisationDTO.java create mode 100644 src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImpl.java create mode 100644 src/main/resources/db/migration/V20260911130000__add_organisation_key.sql create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverterTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationKeyRepositoryTest.java create mode 100644 src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImplTest.java diff --git a/docs/DATABASE_SCHEMA.md b/docs/DATABASE_SCHEMA.md index 3a93db7..37e9ff1 100644 --- a/docs/DATABASE_SCHEMA.md +++ b/docs/DATABASE_SCHEMA.md @@ -46,6 +46,7 @@ erDiagram ORGANISATION { BIGSERIAL id PK VARCHAR name + VARCHAR organisation_key UK BOOLEAN certificate_automation_enabled } PRODUCER { @@ -178,10 +179,15 @@ Represents an organisation that owns Producers and Consumers. Columns: - `id` BIGSERIAL, primary key - `name` VARCHAR(150), not null +- `organisation_key` VARCHAR(50), not null — stable, human-readable identifier for the organisation (e.g. `ENV`, `BCC`, `HEG`), so callers can address an organisation without depending on ids that differ between environments - `certificate_automation_enabled` BOOLEAN, not null, default TRUE +Indexes and constraints: +- UNIQUE on `organisation_key` (`uq_organisation__organisation_key`) + Usage: - Parent entity for `producer`, `consumer`, and `organisation_certificate`. +- `organisation_key` is exposed as `organisation.key` on the producer and consumer configuration APIs. --- diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverter.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverter.java new file mode 100644 index 0000000..870a7d1 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverter.java @@ -0,0 +1,46 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.converter.impl; + +import org.springframework.stereotype.Component; +import uk.gov.dbt.ndtp.ia.node.management.converter.EntityDtoConverter; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.OrganisationDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; + +/** + * Converter for {@link Organisation} entity and {@link OrganisationDTO}. + * + *

Policy attributes are not mapped here: they live in a separate schema reached through + * {@code PolicyAttributeService}, so the caller assembling the response attaches them. + */ +@Component +public class OrganisationConverter implements EntityDtoConverter { + + @Override + public OrganisationDTO toDto(Organisation entity) { + if (entity == null) { + return null; + } + + return OrganisationDTO.builder() + .name(entity.getName()) + .key(entity.getOrganisationKey()) + .build(); + } + + @Override + public Organisation toEntity(OrganisationDTO dto) { + if (dto == null) { + return null; + } + + Organisation entity = new Organisation(); + entity.setName(dto.getName()); + entity.setOrganisationKey(dto.getKey()); + return entity; + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ConsumerDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ConsumerDTO.java index f319f28..ec019a6 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ConsumerDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ConsumerDTO.java @@ -41,5 +41,11 @@ public class ConsumerDTO { private final List attributes = new ArrayList<>(); private final List policyAttributes = new ArrayList<>(); - private final List organisationPolicyAttributes = new ArrayList<>(); + + /** + * The organisation this consumer belongs to, including its key and policy attributes. Replaces + * the former flat {@code organisationPolicyAttributes} list, which carried the same attributes + * with no way to tell which organisation they described. + */ + private OrganisationDTO organisation; } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/OrganisationDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/OrganisationDTO.java new file mode 100644 index 0000000..36fe682 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/OrganisationDTO.java @@ -0,0 +1,38 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.model.dto; + +import java.util.ArrayList; +import java.util.List; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; + +/** + * The organisation a producer or consumer belongs to, as exposed on the configuration APIs. + * + *

Carries {@code key} rather than the database id: the key is stable, readable, and unique, + * so a federator can match on it without depending on ids that differ between environments. + */ +@Builder +@Getter +@Setter +@NoArgsConstructor +@AllArgsConstructor +public class OrganisationDTO { + + /** The organisation's display name (e.g. {@code "Environment Agency (ENV)"}). */ + private String name; + + /** The organisation's unique key (e.g. {@code "ENV"}). */ + private String key; + + /** Live {@code ORGANISATION}-scope policy attributes for this organisation. */ + private final List policyAttributes = new ArrayList<>(); +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerConfigDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerConfigDTO.java index e2a2c57..34d06e4 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerConfigDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerConfigDTO.java @@ -1,6 +1,6 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ @@ -14,5 +14,13 @@ @Getter public class ProducerConfigDTO { private String clientId; + + /** + * The organisation the requesting client's producers belong to, including its key and + * {@code ORGANISATION}-scope policy attributes. Null when no producer resolved an + * organisation; where producers somehow span more than one, the first is used. + */ + private OrganisationDTO organisation; + private List producers; } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerDTO.java index 92c5b46..5e55164 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProducerDTO.java @@ -42,5 +42,8 @@ public class ProducerDTO { private Boolean tls; private String idpClientId; + /** The organisation this producer belongs to, including its key and policy attributes. */ + private OrganisationDTO organisation; + private final List policyAttributes = new ArrayList<>(); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDTO.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDTO.java index 0480026..29c1b72 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDTO.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/ProductDTO.java @@ -1,6 +1,6 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ @@ -35,7 +35,16 @@ public class ProductDTO { private String source; + // @Builder.Default on each list: without it the generated builder bypasses these initialisers + // and hands back nulls, which is why callers used to have to null-check getConsumers(). + + @Builder.Default private List consumers = new ArrayList<>(); + @Builder.Default private List configurations = new ArrayList<>(); + + /** Live {@code PRODUCT}-scope policy attributes for this product. */ + @Builder.Default + private List policyAttributes = new ArrayList<>(); } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/Organisation.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/Organisation.java index f433bfe..2f06acf 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/Organisation.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/entity/Organisation.java @@ -1,6 +1,6 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ @@ -23,6 +23,13 @@ public class Organisation { @Column(name = "name", nullable = false, length = 150) private String name; + /** + * Stable, human-readable identifier for the organisation (e.g. {@code ENV}), unique across + * organisations and indexed, so callers can address an organisation without knowing its id. + */ + @Column(name = "organisation_key", nullable = false, unique = true, length = 50) + private String organisationKey; + @Column(name = "certificate_automation_enabled", nullable = false) private Boolean certificateAutomationEnabled = true; } diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationRepository.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationRepository.java index 4439596..f775839 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationRepository.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationRepository.java @@ -1,11 +1,12 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; +import java.util.Optional; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.stereotype.Repository; import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; @@ -21,4 +22,22 @@ * Primary focus is on the {@link Organisation} entity with the identifier type {@link Long}. */ @Repository -public interface OrganisationRepository extends JpaRepository {} +public interface OrganisationRepository extends JpaRepository { + + /** + * Finds an organisation by its unique key (e.g. {@code ENV}). + * + * @param organisationKey the organisation key to look up + * @return the matching organisation, or empty when no organisation carries that key + */ + Optional findByOrganisationKey(String organisationKey); + + /** + * Whether any organisation already carries the given key. The column is unique, so this is + * the cheap way to check before writing rather than catching a constraint violation. + * + * @param organisationKey the organisation key to check + * @return true when the key is already taken + */ + boolean existsByOrganisationKey(String organisationKey); +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/OrganisationService.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/OrganisationService.java index 8452bcb..255153e 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/OrganisationService.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/OrganisationService.java @@ -1,12 +1,43 @@ /* * SPDX-License-Identifier: Apache-2.0 - * © Crown Copyright 2025. This work has been developed by the National Digital Twin Programme and is legally + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally * attributed to the Department for Business and Trade (UK) as the governing entity. */ package uk.gov.dbt.ndtp.ia.node.management.service.data; +import java.util.Collection; +import java.util.Map; +import java.util.Optional; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.OrganisationDTO; + /** * Service interface for managing Organisation entities. */ -public interface OrganisationService {} +public interface OrganisationService { + + /** + * Finds an organisation by its database id. + * + * @param id the organisation id + * @return the organisation, or empty when no organisation has that id + */ + Optional findById(Long id); + + /** + * Finds an organisation by its unique key (e.g. {@code ENV}). + * + * @param organisationKey the organisation key + * @return the organisation, or empty when no organisation carries that key + */ + Optional findByKey(String organisationKey); + + /** + * Finds several organisations at once, keyed by id - one query rather than one per id, for + * callers assembling a response that mentions the same organisations repeatedly. + * + * @param ids the organisation ids to look up; null or empty yields an empty map + * @return the organisations found, keyed by id. Ids with no matching row are absent. + */ + Map findByIds(Collection ids); +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScope.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScope.java index bb5adcf..3aed32c 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScope.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScope.java @@ -7,14 +7,16 @@ package uk.gov.dbt.ndtp.ia.node.management.service.data; /** - * The {@code policy_attribute_scope.code} values this change resolves policy attributes for, on {@code - * GET /api/v1/configuration/producer}: the producer itself, each allowed consumer, each of those - * consumers' organisations, and each subscription ({@code product_consumer}). Not a general - * registry of every {@code policy_attribute_scope} row (e.g. {@code PRODUCT} is seeded but out of scope - * for this change - see design.md). + * The {@code policy_attribute_scope.code} values policy attributes are resolved for on {@code + * GET /api/v1/configuration/producer}: the producer itself, each of its products, each allowed + * consumer, the organisations those belong to, and each subscription ({@code product_consumer}). + * + *

This now covers every seeded {@code policy_attribute_scope} row; {@link #code()} is verified + * against the seeded codes in {@code PolicyAttributeScopeTest}. */ public enum PolicyAttributeScope { PRODUCER("PRODUCER"), + PRODUCT("PRODUCT"), CONSUMER("CONSUMER"), ORGANISATION("ORGANISATION"), SUBSCRIPTION("SUBSCRIPTION"); diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImpl.java new file mode 100644 index 0000000..3fa0bf5 --- /dev/null +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImpl.java @@ -0,0 +1,65 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.data.impl; + +import java.util.Collection; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Optional; +import org.springframework.stereotype.Service; +import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationConverter; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.OrganisationDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.OrganisationRepository; +import uk.gov.dbt.ndtp.ia.node.management.service.data.OrganisationService; + +/** + * Reads organisations for callers that need an organisation's name and key without holding an + * entity - notably the configuration APIs, which run outside a transaction and so cannot follow + * the lazy {@code producer.org}/{@code consumer.org} associations. + */ +@Service +public class OrganisationServiceImpl implements OrganisationService { + + private final OrganisationRepository organisationRepository; + private final OrganisationConverter organisationConverter; + + public OrganisationServiceImpl( + OrganisationRepository organisationRepository, OrganisationConverter organisationConverter) { + this.organisationRepository = organisationRepository; + this.organisationConverter = organisationConverter; + } + + @Override + public Optional findById(Long id) { + if (id == null) { + return Optional.empty(); + } + return organisationRepository.findById(id).map(organisationConverter::toDto); + } + + @Override + public Optional findByKey(String organisationKey) { + if (organisationKey == null || organisationKey.isBlank()) { + return Optional.empty(); + } + return organisationRepository.findByOrganisationKey(organisationKey).map(organisationConverter::toDto); + } + + @Override + public Map findByIds(Collection ids) { + if (ids == null || ids.isEmpty()) { + return Map.of(); + } + + Map byId = new LinkedHashMap<>(); + for (Organisation organisation : organisationRepository.findAllById(ids)) { + byId.put(organisation.getId(), organisationConverter.toDto(organisation)); + } + return byId; + } +} diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java index c66c815..e389343 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java @@ -13,14 +13,19 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; +import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.OrganisationDTO; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; +import uk.gov.dbt.ndtp.ia.node.management.service.data.OrganisationService; import uk.gov.dbt.ndtp.ia.node.management.service.data.PolicyAttributeScope; import uk.gov.dbt.ndtp.ia.node.management.service.data.PolicyAttributeService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; @@ -31,6 +36,7 @@ * Implementation of {@link ConfigurationProvider} that retrieves configuration from database services. */ @Service +@Slf4j public class ConfigurationProviderImpl implements ConfigurationProvider { private final ConsumerService consumerService; @@ -43,6 +49,8 @@ public class ConfigurationProviderImpl implements ConfigurationProvider { private final PolicyAttributeService policyAttributeService; + private final OrganisationService organisationService; + /** * Constructs a new ConfigurationProviderImpl with required services. * @@ -51,19 +59,22 @@ public class ConfigurationProviderImpl implements ConfigurationProvider { * @param producerService the producer service * @param certificateValidationProvider the certificate validation provider * @param policyAttributeService resolves policy attributes for the producer config response + * @param organisationService resolves the organisation carried by each producer and consumer */ public ConfigurationProviderImpl( ConsumerService consumerService, ProductConsumerService consumerAllowedDataProviders, ProducerService producerService, CertificateValidationProvider certificateValidationProvider, - PolicyAttributeService policyAttributeService) { + PolicyAttributeService policyAttributeService, + OrganisationService organisationService) { this.consumerService = consumerService; this.productConsumerService = consumerAllowedDataProviders; this.producerService = producerService; this.certificateValidationProvider = certificateValidationProvider; this.policyAttributeService = policyAttributeService; + this.organisationService = organisationService; } /** @@ -125,6 +136,10 @@ public ConsumerConfigDTO getConsumerConfigByClientId(String clientId, Optional producers) { .addAll(policyAttributeService.findAttributes(producer.getId(), PolicyAttributeScope.PRODUCER)); for (ProductDTO product : producer.getProducts()) { + product.getPolicyAttributes() + .addAll(policyAttributeService.findAttributes(product.getId(), PolicyAttributeScope.PRODUCT)); + for (ConsumerDTO consumer : product.getConsumers()) { consumer.getPolicyAttributes() .addAll(policyAttributeService.findAttributes( consumer.getId(), PolicyAttributeScope.CONSUMER)); - consumer.getOrganisationPolicyAttributes() - .addAll(policyAttributeService.findAttributes( - consumer.getOrgId(), PolicyAttributeScope.ORGANISATION)); } for (ProductConsumerDTO configuration : product.getConfigurations()) { configuration @@ -277,6 +294,124 @@ private void populatePolicyAttributes(List producers) { } } + /** + * Attaches an {@link OrganisationDTO} - name, unique key, and {@code ORGANISATION}-scope policy + * attributes - to every producer and to every consumer nested under their products. + * + *

Organisations are read through {@link OrganisationService} rather than off the entity: the + * {@code producer.org}/{@code consumer.org} associations are lazy and this runs outside a + * transaction. Each distinct organisation is fetched once and its attributes resolved once, then + * a separate DTO instance is handed to each producer/consumer so nothing is shared by reference. + * + * @param producers the assembled producer graph + * @param includePolicyAttributes whether to attach each organisation's {@code ORGANISATION}-scope + * policy attributes. False on the consumer config response, which names the producers' + * organisations but must not disclose what those organisations are entitled to hold; when + * false, no attribute lookup is performed at all. + */ + private void populateOrganisations(List producers, boolean includePolicyAttributes) { + Set orgIds = new LinkedHashSet<>(); + for (ProducerDTO producer : producers) { + if (producer.getOrgId() != null) { + orgIds.add(producer.getOrgId()); + } + for (ProductDTO product : producer.getProducts()) { + // The consumer-config path never resolves consumers onto its products, so this is + // null there rather than an empty list. + for (ConsumerDTO consumer : consumersOf(product)) { + if (consumer.getOrgId() != null) { + orgIds.add(consumer.getOrgId()); + } + } + } + } + + if (orgIds.isEmpty()) { + return; + } + + Map organisationsById = organisationService.findByIds(orgIds); + Map> attributesByOrgId = new LinkedHashMap<>(); + if (includePolicyAttributes) { + for (Long orgId : organisationsById.keySet()) { + attributesByOrgId.put( + orgId, policyAttributeService.findAttributes(orgId, PolicyAttributeScope.ORGANISATION)); + } + } + + for (ProducerDTO producer : producers) { + producer.setOrganisation(organisationFor(producer.getOrgId(), organisationsById, attributesByOrgId)); + for (ProductDTO product : producer.getProducts()) { + for (ConsumerDTO consumer : consumersOf(product)) { + consumer.setOrganisation( + organisationFor(consumer.getOrgId(), organisationsById, attributesByOrgId)); + } + } + } + } + + /** + * The organisation to report at the top of a producer config response: the one its producers + * belong to. They are all the requesting client's producers, so in practice they share an + * organisation; if they ever do not, the first is used and the disagreement logged rather than + * silently picking one. + * + * @param producers the assembled producer graph, after {@link #populateOrganisations} + * @return a copy of the organisation, or null when no producer resolved one + */ + private OrganisationDTO configOrganisation(List producers) { + List resolved = producers.stream() + .map(ProducerDTO::getOrganisation) + .filter(Objects::nonNull) + .toList(); + + if (resolved.isEmpty()) { + return null; + } + + long distinctKeys = + resolved.stream().map(OrganisationDTO::getKey).distinct().count(); + if (distinctKeys > 1) { + log.warn( + "Producers for one client span {} organisations; reporting {} on the config response", + distinctKeys, + resolved.getFirst().getKey()); + } + + OrganisationDTO first = resolved.getFirst(); + OrganisationDTO organisation = OrganisationDTO.builder() + .name(first.getName()) + .key(first.getKey()) + .build(); + organisation.getPolicyAttributes().addAll(first.getPolicyAttributes()); + return organisation; + } + + private List consumersOf(ProductDTO product) { + return product.getConsumers() == null ? List.of() : product.getConsumers(); + } + + /** + * Builds a fresh {@link OrganisationDTO} for one owner, or null when the organisation could not + * be resolved (an orphaned {@code org_id}, which the response should simply omit). + */ + private OrganisationDTO organisationFor( + Long orgId, + Map organisationsById, + Map> attributesByOrgId) { + OrganisationDTO resolved = orgId == null ? null : organisationsById.get(orgId); + if (resolved == null) { + return null; + } + + OrganisationDTO organisation = OrganisationDTO.builder() + .name(resolved.getName()) + .key(resolved.getKey()) + .build(); + organisation.getPolicyAttributes().addAll(attributesByOrgId.getOrDefault(orgId, List.of())); + return organisation; + } + /** * Checks if a provider (product consumer) is valid based on its granted date and validity period. * diff --git a/src/main/resources/db/migration/V20260911130000__add_organisation_key.sql b/src/main/resources/db/migration/V20260911130000__add_organisation_key.sql new file mode 100644 index 0000000..eeed219 --- /dev/null +++ b/src/main/resources/db/migration/V20260911130000__add_organisation_key.sql @@ -0,0 +1,35 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +-- A stable, human-readable key for an organisation, for callers that should not have to know +-- database ids: ENV, BCC, HEG. Unique and indexed, so it can be used as a lookup key. + +ALTER TABLE organisation ADD COLUMN organisation_key VARCHAR(50); + +-- Backfill from the existing name. The sample organisations carry their key in a trailing +-- parenthesised code ("Environment Agency (ENV)" -> ENV); anything else falls back to an +-- upper-snake-case slug of the name, so a real deployment's rows get a usable key too. +UPDATE organisation +SET organisation_key = left( + upper(coalesce( + substring(name from '\(([A-Za-z0-9_-]+)\)\s*$'), + regexp_replace(btrim(name), '[^A-Za-z0-9]+', '_', 'g'))), + 45); + +-- Two organisations whose names slug to the same key would break the unique index below, so +-- disambiguate the later row(s) by id rather than failing the migration. +UPDATE organisation o +SET organisation_key = o.organisation_key || '_' || o.id +WHERE EXISTS ( + SELECT 1 FROM organisation earlier + WHERE earlier.organisation_key = o.organisation_key + AND earlier.id < o.id); + +-- A row with a blank name would have slugged to an empty string; give it something addressable. +UPDATE organisation SET organisation_key = 'ORG_' || id WHERE coalesce(organisation_key, '') = ''; + +ALTER TABLE organisation ALTER COLUMN organisation_key SET NOT NULL; +CREATE UNIQUE INDEX uq_organisation__organisation_key ON organisation (organisation_key); diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverterTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverterTest.java new file mode 100644 index 0000000..7dd0e50 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/converter/impl/OrganisationConverterTest.java @@ -0,0 +1,69 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.converter.impl; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import org.junit.jupiter.api.Test; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.OrganisationDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; + +class OrganisationConverterTest { + + private final OrganisationConverter converter = new OrganisationConverter(); + + private static Organisation entity(String name, String key) { + Organisation organisation = new Organisation(); + organisation.setName(name); + organisation.setOrganisationKey(key); + return organisation; + } + + @Test + void toDto_mapsNameAndKey() { + OrganisationDTO dto = converter.toDto(entity("Environment Agency (ENV)", "ENV")); + + assertThat(dto.getName()).isEqualTo("Environment Agency (ENV)"); + assertThat(dto.getKey()).isEqualTo("ENV"); + } + + @Test + void toDto_leavesPolicyAttributesEmptyForTheCallerToPopulate() { + assertThat(converter.toDto(entity("Homes England (HEG)", "HEG")).getPolicyAttributes()) + .isEmpty(); + } + + @Test + void toDto_returnsNullForNullEntity() { + assertThat(converter.toDto(null)).isNull(); + } + + @Test + void toEntity_mapsNameAndKey() { + Organisation organisation = converter.toEntity(OrganisationDTO.builder() + .name("Bristol City Council (BCC)") + .key("BCC") + .build()); + + assertThat(organisation.getName()).isEqualTo("Bristol City Council (BCC)"); + assertThat(organisation.getOrganisationKey()).isEqualTo("BCC"); + } + + @Test + void toEntity_returnsNullForNullDto() { + assertThat(converter.toEntity(null)).isNull(); + } + + @Test + void toDtoList_mapsEveryEntity() { + List dtos = converter.toDtoList( + List.of(entity("Environment Agency (ENV)", "ENV"), entity("Homes England (HEG)", "HEG"))); + + assertThat(dtos).extracting(OrganisationDTO::getKey).containsExactly("ENV", "HEG"); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java index 764c49a..70fb275 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/model/dto/PolicyAttributeFieldsSerializationTest.java @@ -32,6 +32,37 @@ void policyAttribute_serialisesNamespaceNameAndValueOnly() throws Exception { assertThat(json).isEqualTo("{\"namespace\":\"policy\",\"name\":\"risk-tier\",\"value\":\"gold\"}"); } + @Test + void organisationDto_serialisesNameKeyAndPolicyAttributes() throws Exception { + OrganisationDTO organisation = OrganisationDTO.builder() + .name("Environment Agency (ENV)") + .key("ENV") + .build(); + organisation + .getPolicyAttributes() + .add(PolicyAttributeDTO.builder() + .namespace("policy") + .name("jurisdictions") + .value("England") + .build()); + + JsonNode json = objectMapper.readTree(objectMapper.writeValueAsString(organisation)); + + assertThat(json.get("name").asText()).isEqualTo("Environment Agency (ENV)"); + assertThat(json.get("key").asText()).isEqualTo("ENV"); + assertThat(json.get("policyAttributes")).hasSize(1); + assertThat(json.get("policyAttributes").get(0).get("name").asText()).isEqualTo("jurisdictions"); + } + + @Test + void organisationDto_policyAttributesSerialisesAsEmptyArrayWhenUnpopulated() throws Exception { + JsonNode json = objectMapper.readTree( + objectMapper.writeValueAsString(OrganisationDTO.builder().build())); + + assertThat(json.get("policyAttributes").isArray()).isTrue(); + assertThat(json.get("policyAttributes")).isEmpty(); + } + @Test void producerDto_policyAttributesSerialisesAsEmptyArray() throws Exception { JsonNode json = objectMapper.readTree( @@ -49,8 +80,35 @@ void consumerDto_policyAttributeFieldsSerialiseAsEmptyArrays() throws Exception assertThat(json.get("policyAttributes").isArray()).isTrue(); assertThat(json.get("policyAttributes")).isEmpty(); - assertThat(json.get("organisationPolicyAttributes").isArray()).isTrue(); - assertThat(json.get("organisationPolicyAttributes")).isEmpty(); + // organisation is a whole DTO now, not a flat attribute list, and is null until resolved + assertThat(json.has("organisation")).isTrue(); + assertThat(json.get("organisation").isNull()).isTrue(); + } + + @Test + void productDto_policyAttributesSerialisesAsEmptyArray() throws Exception { + JsonNode json = objectMapper.readTree(objectMapper.writeValueAsString(new ProductDTO())); + + assertThat(json.has("policyAttributes")).isTrue(); + assertThat(json.get("policyAttributes").isArray()).isTrue(); + assertThat(json.get("policyAttributes")).isEmpty(); + } + + @Test + void producerConfigDto_carriesOrganisation() throws Exception { + ProducerConfigDTO config = ProducerConfigDTO.builder() + .clientId("FEDERATOR_ENV") + .organisation(OrganisationDTO.builder() + .name("Environment Agency (ENV)") + .key("ENV") + .build()) + .build(); + + JsonNode json = objectMapper.readTree(objectMapper.writeValueAsString(config)); + + assertThat(json.get("organisation").get("key").asText()).isEqualTo("ENV"); + assertThat(json.get("organisation").get("name").asText()).isEqualTo("Environment Agency (ENV)"); + assertThat(json.get("organisation").get("policyAttributes").isArray()).isTrue(); } @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java index 525793c..711a3f8 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/AttributeValueSoftDeleteTriggerTest.java @@ -11,6 +11,7 @@ import java.math.BigDecimal; import java.sql.Timestamp; import java.time.Instant; +import java.util.UUID; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.orm.jpa.TestEntityManager; @@ -95,6 +96,9 @@ private Long persistLiveValue(AttributeDefinitionScope binding, Long entityId) { private Organisation persistOrganisation() { Organisation organisation = new Organisation(); organisation.setName("Trigger Test Org"); + // organisation_key is NOT NULL and unique; each test persists its own organisation, so the + // key has to be unique per call rather than a fixed literal. + organisation.setOrganisationKey("TRIG_" + UUID.randomUUID().toString().substring(0, 8)); return organisationRepository.saveAndFlush(organisation); } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationKeyRepositoryTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationKeyRepositoryTest.java new file mode 100644 index 0000000..a62bd5e --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/persistency/repository/OrganisationKeyRepositoryTest.java @@ -0,0 +1,87 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.persistency.repository; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.dao.DataIntegrityViolationException; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; + +/** + * Covers {@code organisation.organisation_key} against real Postgres: the finders, and the + * NOT NULL and unique constraints the migration puts on the column. H2 would not prove the + * constraint behaviour the migration actually creates, hence the Postgres-backed base class. + */ +class OrganisationKeyRepositoryTest extends AbstractPostgresRepositoryTest { + + @Autowired + private OrganisationRepository organisationRepository; + + private Organisation persist(String name, String key) { + Organisation organisation = new Organisation(); + organisation.setName(name); + organisation.setOrganisationKey(key); + return organisationRepository.saveAndFlush(organisation); + } + + @Test + void findByOrganisationKey_returnsTheMatchingOrganisation() { + Organisation saved = persist("Environment Agency (ENV)", "KEY_FIND_ENV"); + + assertThat(organisationRepository.findByOrganisationKey("KEY_FIND_ENV")) + .isPresent() + .get() + .satisfies(found -> { + assertThat(found.getId()).isEqualTo(saved.getId()); + assertThat(found.getName()).isEqualTo("Environment Agency (ENV)"); + assertThat(found.getOrganisationKey()).isEqualTo("KEY_FIND_ENV"); + }); + } + + @Test + void findByOrganisationKey_returnsEmptyForAnUnknownKey() { + assertThat(organisationRepository.findByOrganisationKey("KEY_NOT_PRESENT")) + .isEmpty(); + } + + @Test + void findByOrganisationKey_isCaseSensitive() { + persist("Homes England (HEG)", "KEY_CASE_HEG"); + + assertThat(organisationRepository.findByOrganisationKey("key_case_heg")).isEmpty(); + } + + @Test + void existsByOrganisationKey_reflectsWhetherTheKeyIsTaken() { + persist("Bristol City Council (BCC)", "KEY_EXISTS_BCC"); + + assertThat(organisationRepository.existsByOrganisationKey("KEY_EXISTS_BCC")) + .isTrue(); + assertThat(organisationRepository.existsByOrganisationKey("KEY_EXISTS_NOBODY")) + .isFalse(); + } + + @Test + void organisationKey_isUnique() { + persist("First org", "KEY_DUPLICATE"); + + assertThatThrownBy(() -> persist("Second org", "KEY_DUPLICATE")) + .isInstanceOf(DataIntegrityViolationException.class); + } + + @Test + void organisationKey_isMandatory() { + Organisation noKey = new Organisation(); + noKey.setName("Org with no key"); + + assertThatThrownBy(() -> organisationRepository.saveAndFlush(noKey)) + .isInstanceOf(DataIntegrityViolationException.class); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java index ad6447d..6959a2b 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java @@ -32,9 +32,14 @@ void code_namesASeededAttributeScopeRow(PolicyAttributeScope scope) { } @Test - void doesNotIncludeProductScope() { + void coversEverySeededScopeCode() { assertThat(PolicyAttributeScope.values()) .extracting(PolicyAttributeScope::code) - .doesNotContain("PRODUCT"); + .containsExactlyInAnyOrder("PRODUCER", "PRODUCT", "CONSUMER", "ORGANISATION", "SUBSCRIPTION"); + assertThat(attributeScopeRepository.findAll()) + .extracting(scope -> scope.getCode()) + .containsExactlyInAnyOrderElementsOf(java.util.Arrays.stream(PolicyAttributeScope.values()) + .map(PolicyAttributeScope::code) + .toList()); } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImplTest.java new file mode 100644 index 0000000..10068e6 --- /dev/null +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/impl/OrganisationServiceImplTest.java @@ -0,0 +1,135 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * © Crown Copyright 2026. This work has been developed by the National Digital Twin Programme and is legally + * attributed to the Department for Business and Trade (UK) as the governing entity. + */ + +package uk.gov.dbt.ndtp.ia.node.management.service.data.impl; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.util.List; +import java.util.Optional; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationConverter; +import uk.gov.dbt.ndtp.ia.node.management.model.dto.OrganisationDTO; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.Organisation; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.OrganisationRepository; + +@ExtendWith(MockitoExtension.class) +class OrganisationServiceImplTest { + + @Mock + private OrganisationRepository organisationRepository; + + private OrganisationServiceImpl service; + + @BeforeEach + void setUp() { + service = new OrganisationServiceImpl(organisationRepository, new OrganisationConverter()); + } + + private static Organisation organisation(Long id, String name, String key) { + Organisation organisation = new Organisation(); + organisation.setId(id); + organisation.setName(name); + organisation.setOrganisationKey(key); + return organisation; + } + + @Test + void findById_mapsNameAndKey() { + when(organisationRepository.findById(1L)) + .thenReturn(Optional.of(organisation(1L, "Environment Agency (ENV)", "ENV"))); + + Optional result = service.findById(1L); + + assertThat(result).isPresent(); + assertThat(result.get().getName()).isEqualTo("Environment Agency (ENV)"); + assertThat(result.get().getKey()).isEqualTo("ENV"); + assertThat(result.get().getPolicyAttributes()).isEmpty(); + } + + @Test + void findById_returnsEmptyWhenNotFound() { + when(organisationRepository.findById(99L)).thenReturn(Optional.empty()); + + assertThat(service.findById(99L)).isEmpty(); + } + + @Test + void findById_returnsEmptyForNullIdWithoutQuerying() { + assertThat(service.findById(null)).isEmpty(); + + verifyNoInteractions(organisationRepository); + } + + @Test + void findByKey_mapsNameAndKey() { + when(organisationRepository.findByOrganisationKey("BCC")) + .thenReturn(Optional.of(organisation(2L, "Bristol City Council (BCC)", "BCC"))); + + Optional result = service.findByKey("BCC"); + + assertThat(result).isPresent(); + assertThat(result.get().getKey()).isEqualTo("BCC"); + assertThat(result.get().getName()).isEqualTo("Bristol City Council (BCC)"); + } + + @Test + void findByKey_returnsEmptyWhenNotFound() { + when(organisationRepository.findByOrganisationKey("NOPE")).thenReturn(Optional.empty()); + + assertThat(service.findByKey("NOPE")).isEmpty(); + } + + @Test + void findByKey_returnsEmptyForNullOrBlankWithoutQuerying() { + assertThat(service.findByKey(null)).isEmpty(); + assertThat(service.findByKey("")).isEmpty(); + assertThat(service.findByKey(" ")).isEmpty(); + + verify(organisationRepository, never()).findByOrganisationKey(any()); + } + + @Test + void findByIds_returnsOneEntryPerFoundOrganisationKeyedById() { + when(organisationRepository.findAllById(List.of(1L, 3L))) + .thenReturn(List.of( + organisation(1L, "Environment Agency (ENV)", "ENV"), + organisation(3L, "Homes England (HEG)", "HEG"))); + + var result = service.findByIds(List.of(1L, 3L)); + + assertThat(result).hasSize(2); + assertThat(result.get(1L).getKey()).isEqualTo("ENV"); + assertThat(result.get(3L).getKey()).isEqualTo("HEG"); + } + + @Test + void findByIds_omitsIdsWithNoMatchingRow() { + when(organisationRepository.findAllById(List.of(1L, 404L))) + .thenReturn(List.of(organisation(1L, "Environment Agency (ENV)", "ENV"))); + + var result = service.findByIds(List.of(1L, 404L)); + + assertThat(result).containsOnlyKeys(1L); + } + + @Test + void findByIds_returnsEmptyMapForNullOrEmptyWithoutQuerying() { + assertThat(service.findByIds(null)).isEmpty(); + assertThat(service.findByIds(List.of())).isEmpty(); + + verifyNoInteractions(organisationRepository); + } +} diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index 1b63cf3..471e88f 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -27,6 +27,7 @@ import org.mockito.MockitoAnnotations; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; +import uk.gov.dbt.ndtp.ia.node.management.service.data.OrganisationService; import uk.gov.dbt.ndtp.ia.node.management.service.data.PolicyAttributeScope; import uk.gov.dbt.ndtp.ia.node.management.service.data.PolicyAttributeService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; @@ -50,6 +51,9 @@ class ConfigurationProviderImplTest { @Mock private PolicyAttributeService policyAttributeService; + @Mock + private OrganisationService organisationService; + @InjectMocks private ConfigurationProviderImpl configurationProvider; @@ -61,7 +65,8 @@ void setUp() { productConsumerService, producerService, certificateValidationProvider, - policyAttributeService); + policyAttributeService, + organisationService); // Default: treat all orgs as having active certificates, override in specific // tests to simulate inactive/missing certs. when(certificateValidationProvider.findActiveOrganisationIds(any())).thenAnswer(invocation -> { @@ -377,6 +382,42 @@ void getProducerConfig_filtersOutConsumersWithInactiveCerts() { // DPAV-3162: policy attribute wiring + @Test + void getConsumerConfigByClientId_namesTheProducersOrganisationButWithoutItsPolicyAttributes() { + String clientId = "consumerClient"; + ConsumerDTO consumer = consumer(500L, clientId, "c500", "CRON", "@daily"); + when(consumerService.findByIdpClientId(clientId)).thenReturn(List.of(consumer)); + + ProductConsumerDTO subscription = productConsumer(700L, 500L, null, null); + subscription.setId(9100L); + when(productConsumerService.findByConsumerId(500L)).thenReturn(List.of(subscription)); + + ProductDTO product = product(700L, "prod"); + ProducerDTO producer = producer(80L, true, product); + when(producerService.getProducersByConsumerIds(List.of(500L))).thenReturn(List.of(producer)); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 1L, + OrganisationDTO.builder() + .name("Producer Org") + .key("PROD_ORG") + .build())); + + ConsumerConfigDTO cfg = configurationProvider.getConsumerConfigByClientId(clientId, Optional.empty()); + + ProducerDTO returned = cfg.getProducers().get(0); + // the consumer is told which organisation publishes to it... + assertThat(returned.getOrganisation()).isNotNull(); + assertThat(returned.getOrganisation().getName()).isEqualTo("Producer Org"); + assertThat(returned.getOrganisation().getKey()).isEqualTo("PROD_ORG"); + // ...but never what that organisation is entitled to hold + assertThat(returned.getOrganisation().getPolicyAttributes()).isEmpty(); + assertThat(returned.getPolicyAttributes()).isEmpty(); + assertThat(returned.getProducts().get(0).getPolicyAttributes()).isEmpty(); + // no policy attribute lookup happens at all on this path + verifyNoInteractions(policyAttributeService); + } + @Test void getProducerConfigByClientId_populatesPolicyAttributesForEveryScope() { String clientId = "policyClient"; @@ -413,6 +454,18 @@ void getProducerConfigByClientId_populatesPolicyAttributesForEveryScope() { .name("d") .value("4") .build(); + PolicyAttributeDTO productAttr = PolicyAttributeDTO.builder() + .namespace("policy") + .name("e") + .value("5") + .build(); + // the producer's own organisation (id 1) - distinct from the consumer's organisation (801), + // so the assertions below prove which one the config-level organisation reports + PolicyAttributeDTO producerOrgAttr = PolicyAttributeDTO.builder() + .namespace("policy") + .name("f") + .value("6") + .build(); when(policyAttributeService.findAttributes(70L, PolicyAttributeScope.PRODUCER)) .thenReturn(List.of(producerAttr)); when(policyAttributeService.findAttributes(701L, PolicyAttributeScope.CONSUMER)) @@ -421,6 +474,22 @@ void getProducerConfigByClientId_populatesPolicyAttributesForEveryScope() { .thenReturn(List.of(orgAttr)); when(policyAttributeService.findAttributes(9001L, PolicyAttributeScope.SUBSCRIPTION)) .thenReturn(List.of(subscriptionAttr)); + when(policyAttributeService.findAttributes(700L, PolicyAttributeScope.PRODUCT)) + .thenReturn(List.of(productAttr)); + when(policyAttributeService.findAttributes(1L, PolicyAttributeScope.ORGANISATION)) + .thenReturn(List.of(producerOrgAttr)); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 1L, + OrganisationDTO.builder() + .name("Producer Org") + .key("PROD_ORG") + .build(), + 801L, + OrganisationDTO.builder() + .name("Consumer Org") + .key("CONS_ORG") + .build())); ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); @@ -430,11 +499,157 @@ void getProducerConfigByClientId_populatesPolicyAttributesForEveryScope() { ConsumerDTO returnedConsumer = returnedProducer.getProducts().get(0).getConsumers().get(0); assertThat(returnedConsumer.getPolicyAttributes()).containsExactly(consumerAttr); - assertThat(returnedConsumer.getOrganisationPolicyAttributes()).containsExactly(orgAttr); + assertThat(returnedConsumer.getOrganisation()).isNotNull(); + assertThat(returnedConsumer.getOrganisation().getPolicyAttributes()).containsExactly(orgAttr); ProductConsumerDTO returnedSubscription = returnedProducer.getProducts().get(0).getConfigurations().get(0); assertThat(returnedSubscription.getPolicyAttributes()).containsExactly(subscriptionAttr); + + assertThat(returnedProducer.getProducts().get(0).getPolicyAttributes()).containsExactly(productAttr); + + // the response itself reports the organisation its producers belong to + assertThat(cfg.getOrganisation()).isNotNull(); + assertThat(cfg.getOrganisation().getKey()).isEqualTo("PROD_ORG"); + assertThat(cfg.getOrganisation().getName()).isEqualTo("Producer Org"); + assertThat(cfg.getOrganisation().getPolicyAttributes()).containsExactly(producerOrgAttr); + // and the producer carries the same organisation as the response header + assertThat(returnedProducer.getOrganisation().getKey()).isEqualTo("PROD_ORG"); + } + + @Test + void getProducerConfigByClientId_configOrganisationIsNullWhenNoProducerResolvesOne() { + String clientId = "noConfigOrgClient"; + ProductDTO product = product(700L, "prod"); + ProducerDTO producer = producer(75L, true, product); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of()); + when(organisationService.findByIds(any())).thenReturn(Map.of()); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + assertThat(cfg.getOrganisation()).isNull(); + } + + @Test + void getProducerConfigByClientId_producersSpanningTwoOrganisations_reportsTheFirstAndWarns() { + String clientId = "multiOrgClient"; + ProducerDTO first = producer(77L, true, product(700L, "prodA")); + ProducerDTO second = producer(78L, true, product(701L, "prodB")); + second.setOrgId(2L); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(first, second)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + when(productConsumerService.findByDataProviderId(any())).thenReturn(List.of()); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 1L, + OrganisationDTO.builder().name("First Org").key("FIRST").build(), + 2L, + OrganisationDTO.builder() + .name("Second Org") + .key("SECOND") + .build())); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + assertThat(cfg.getOrganisation().getKey()).isEqualTo("FIRST"); + // each producer still reports its own organisation + assertThat(cfg.getProducers()) + .extracting(p -> p.getOrganisation().getKey()) + .containsExactly("FIRST", "SECOND"); + } + + @Test + void getProducerConfigByClientId_configOrganisationIsACopyNotTheProducersInstance() { + String clientId = "copyOrgClient"; + ProductDTO product = product(700L, "prod"); + ProducerDTO producer = producer(76L, true, product); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of()); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 1L, + OrganisationDTO.builder() + .name("Producer Org") + .key("PROD_ORG") + .build())); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + assertThat(cfg.getOrganisation()).isNotSameAs(cfg.getProducers().get(0).getOrganisation()); + assertThat(cfg.getOrganisation().getKey()) + .isEqualTo(cfg.getProducers().get(0).getOrganisation().getKey()); + } + + @Test + void getProducerConfigByClientId_leavesOrganisationNullWhenNothingHasAnOrgId() { + String clientId = "noOrgClient"; + ProductDTO product = product(700L, "prod"); + ProducerDTO producer = ProducerDTO.builder() + .id(72L) + .active(true) + .idpClientId("cid") + .name("p") + .build(); + producer.getProducts().add(product); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of()); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + assertThat(cfg.getProducers().get(0).getOrganisation()).isNull(); + // nothing to look up, so the organisation lookup is skipped entirely + verify(organisationService, never()).findByIds(any()); + } + + @Test + void getProducerConfigByClientId_leavesOrganisationNullWhenTheOrgIdResolvesToNothing() { + String clientId = "orphanOrgClient"; + ProductDTO product = product(700L, "prod"); + ProducerDTO producer = producer(73L, true, product); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of()); + // producer.orgId is 1L, but no organisation row comes back for it + when(organisationService.findByIds(any())).thenReturn(Map.of()); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + assertThat(cfg.getProducers().get(0).getOrganisation()).isNull(); + } + + @Test + void getProducerConfigByClientId_consumerWithNoOrgId_getsNullOrganisation() { + String clientId = "consumerNoOrgClient"; + ProductDTO product = product(700L, "prod"); + ProducerDTO producer = producer(74L, true, product); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + + ProductConsumerDTO subscription = productConsumer(700L, 705L, null, null); + subscription.setId(9005L); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of(subscription)); + + ConsumerDTO consumer = ConsumerDTO.builder().name("c705").build(); + consumer.setId(705L); + when(consumerService.findById(705L)).thenReturn(Optional.of(consumer)); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 1L, + OrganisationDTO.builder() + .name("Producer Org") + .key("PROD_ORG") + .build())); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + ConsumerDTO returned = + cfg.getProducers().get(0).getProducts().get(0).getConsumers().get(0); + assertThat(returned.getOrganisation()).isNull(); + assertThat(cfg.getProducers().get(0).getOrganisation().getKey()).isEqualTo("PROD_ORG"); } @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java index 45a84df..03cba43 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ProducerConfigPolicyAttributesIntegrationTest.java @@ -18,12 +18,14 @@ import java.sql.Timestamp; import java.time.Instant; import java.util.HashSet; +import java.util.Locale; import java.util.Optional; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Import; import org.springframework.transaction.annotation.Transactional; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ConsumerConverter; +import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationConverter; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.OrganisationProducerConverter; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ProductConsumerConverter; import uk.gov.dbt.ndtp.ia.node.management.converter.impl.ProductConverter; @@ -48,11 +50,13 @@ import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeDefinitionScopeRepository; import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeScopeRepository; import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeValueRepository; +import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.OrganisationRepository; import uk.gov.dbt.ndtp.ia.node.management.service.data.ConsumerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.PolicyAttributeService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProducerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.ProductConsumerService; import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.ConsumerServiceImpl; +import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.OrganisationServiceImpl; import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.PolicyAttributeServiceImpl; import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.ProducerServiceImpl; import uk.gov.dbt.ndtp.ia.node.management.service.data.impl.ProductConsumerServiceImpl; @@ -102,6 +106,9 @@ class ProducerConfigPolicyAttributesIntegrationTest extends AbstractPostgresRepo @Autowired private AttributeScopeRepository attributeScopeRepository; + @Autowired + private OrganisationRepository organisationRepository; + private ConfigurationProviderImpl configurationProvider() { CertificateValidationProvider certificateValidationProvider = mock(CertificateValidationProvider.class); when(certificateValidationProvider.findActiveOrganisationIds(any())) @@ -113,12 +120,14 @@ private ConfigurationProviderImpl configurationProvider() { productConsumerService, producerService, certificateValidationProvider, - policyAttributeService); + policyAttributeService, + new OrganisationServiceImpl(organisationRepository, new OrganisationConverter())); } private Organisation persistOrganisation(String name) { Organisation org = new Organisation(); org.setName(name); + org.setOrganisationKey(name.toUpperCase(Locale.ROOT).replaceAll("[^A-Z0-9]+", "_")); entityManager.persist(org); return org; } @@ -216,6 +225,8 @@ void producerConfig_carriesPolicyAttributesAcrossAllFourScopes_andEmptyForSiblin persistAttribute("PRODUCER", producer.getId(), "producer-tier", "\"gold\""); persistAttribute("CONSUMER", consumer.getId(), "consumer-tier", "\"silver\""); persistAttribute("ORGANISATION", consumerOrg.getId(), "org-region", "\"uk\""); + persistAttribute("ORGANISATION", producerOrg.getId(), "producer-org-region", "\"north\""); + persistAttribute("PRODUCT", product.getId(), "record-unit", "\"property\""); persistAttribute("SUBSCRIPTION", subscription.getId(), "sub-priority", "1"); // A sibling producer with no attributes at all, for the empty-array assertion. It still @@ -237,12 +248,28 @@ void producerConfig_carriesPolicyAttributesAcrossAllFourScopes_andEmptyForSiblin .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) .containsExactly(tuple("policy", "producer-tier", "gold")); + assertThat(producerDto.getOrganisation()).isNotNull(); + assertThat(producerDto.getOrganisation().getName()).isEqualTo("producer-org"); + assertThat(producerDto.getOrganisation().getKey()).isEqualTo("PRODUCER_ORG"); + + assertThat(cfg.getOrganisation()).isNotNull(); + assertThat(cfg.getOrganisation().getName()).isEqualTo("producer-org"); + assertThat(cfg.getOrganisation().getKey()).isEqualTo("PRODUCER_ORG"); + assertThat(cfg.getOrganisation().getPolicyAttributes()) + .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) + .containsExactly(tuple("policy", "producer-org-region", "north")); + ProductDTO productDto = producerDto.getProducts().get(0); + assertThat(productDto.getPolicyAttributes()) + .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) + .containsExactly(tuple("policy", "record-unit", "property")); ConsumerDTO consumerDto = productDto.getConsumers().get(0); assertThat(consumerDto.getPolicyAttributes()) .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) .containsExactly(tuple("policy", "consumer-tier", "silver")); - assertThat(consumerDto.getOrganisationPolicyAttributes()) + assertThat(consumerDto.getOrganisation().getName()).isEqualTo("consumer-org"); + assertThat(consumerDto.getOrganisation().getKey()).isEqualTo("CONSUMER_ORG"); + assertThat(consumerDto.getOrganisation().getPolicyAttributes()) .extracting(PolicyAttributeDTO::getNamespace, PolicyAttributeDTO::getName, PolicyAttributeDTO::getValue) .containsExactly(tuple("policy", "org-region", "uk")); From 9e3c16fd0464e0af4e6748f1506d852996058c5a Mon Sep 17 00:00:00 2001 From: nikan Negaresh Date: Fri, 11 Sep 2026 16:54:27 +0100 Subject: [PATCH 13/15] feat(config): introduce organisation key and associated functionality - Added a stable, human-readable `organisation_key` column to the `organisation` table with unique constraint and migration SQL. - Backfilled keys for existing records using name-based transformations. - Enhanced `Organisation` entity to include `organisationKey` field. - Introduced `OrganisationDTO`, `OrganisationConverter`, and `OrganisationServiceImpl` for exposing organisation data, including the new key. - Updated repositories with new methods for querying by `organisationKey` and checking key existence. - Adjusted DTOs, tests, and configurations to support the new `organisationKey` functionality and related policy attributes. - Updated integration tests and backfilled data validations. --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index d491017..b622cda 100644 --- a/.gitignore +++ b/.gitignore @@ -56,3 +56,6 @@ build/ ### local-only openspec workspace /openspec/ + +### vault unseal keys (local dev) +docker/vault/vault-keys.env From b74bde01c6d2bedccbeef905c127b7e371275788 Mon Sep 17 00:00:00 2001 From: nikan Negaresh Date: Fri, 11 Sep 2026 17:15:38 +0100 Subject: [PATCH 14/15] refactor(config): streamline organisation resolution and improve test coverage - Refactored `populateOrganisations` method for better readability, introducing `OrganisationHolder` to simplify mapping and assignment logic. - Optimized organisation ID collection and resolution to avoid redundant lookups. - Added comprehensive unit tests to validate organisation resolutions, shared references, and policy attributes. --- .../ConfigurationProviderImpl.java | 91 +++++++++----- .../ConfigurationProviderImplTest.java | 113 ++++++++++++++++++ 2 files changed, 176 insertions(+), 28 deletions(-) diff --git a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java index e389343..193af2c 100644 --- a/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java +++ b/src/main/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImpl.java @@ -19,7 +19,9 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Consumer; import java.util.stream.Collectors; +import java.util.stream.Stream; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; import uk.gov.dbt.ndtp.ia.node.management.model.dto.*; @@ -310,43 +312,76 @@ private void populatePolicyAttributes(List producers) { * false, no attribute lookup is performed at all. */ private void populateOrganisations(List producers, boolean includePolicyAttributes) { - Set orgIds = new LinkedHashSet<>(); - for (ProducerDTO producer : producers) { - if (producer.getOrgId() != null) { - orgIds.add(producer.getOrgId()); - } - for (ProductDTO product : producer.getProducts()) { - // The consumer-config path never resolves consumers onto its products, so this is - // null there rather than an empty list. - for (ConsumerDTO consumer : consumersOf(product)) { - if (consumer.getOrgId() != null) { - orgIds.add(consumer.getOrgId()); - } - } - } - } + List holders = organisationHolders(producers); + Set orgIds = holders.stream() + .map(OrganisationHolder::orgId) + .filter(Objects::nonNull) + .collect(Collectors.toCollection(LinkedHashSet::new)); if (orgIds.isEmpty()) { return; } Map organisationsById = organisationService.findByIds(orgIds); + Map> attributesByOrgId = + organisationPolicyAttributes(organisationsById.keySet(), includePolicyAttributes); + + holders.forEach(holder -> holder.assign(organisationFor(holder.orgId(), organisationsById, attributesByOrgId))); + } + + /** + * Every place in the assembled graph that carries an organisation: each producer, then the + * consumers nested under its products, in that order. + * + * @param producers the assembled producer graph + * @return one holder per producer and per nested consumer + */ + private List organisationHolders(List producers) { + return producers.stream().flatMap(this::organisationHoldersOf).toList(); + } + + private Stream organisationHoldersOf(ProducerDTO producer) { + Stream consumers = producer.getProducts().stream() + // The consumer-config path never resolves consumers onto its products, so this is + // null there rather than an empty list. + .flatMap(product -> consumersOf(product).stream()) + .map(consumer -> new OrganisationHolder(consumer.getOrgId(), consumer::setOrganisation)); + + return Stream.concat( + Stream.of(new OrganisationHolder(producer.getOrgId(), producer::setOrganisation)), consumers); + } + + /** + * Resolves the {@code ORGANISATION}-scope policy attributes of each organisation, once per + * organisation. + * + * @param orgIds the distinct organisations that were resolved + * @param includePolicyAttributes when false no lookup is performed at all and the map is empty + * @return attributes keyed by organisation id + */ + private Map> organisationPolicyAttributes( + Set orgIds, boolean includePolicyAttributes) { + if (!includePolicyAttributes) { + return Map.of(); + } + Map> attributesByOrgId = new LinkedHashMap<>(); - if (includePolicyAttributes) { - for (Long orgId : organisationsById.keySet()) { - attributesByOrgId.put( - orgId, policyAttributeService.findAttributes(orgId, PolicyAttributeScope.ORGANISATION)); - } + for (Long orgId : orgIds) { + attributesByOrgId.put( + orgId, policyAttributeService.findAttributes(orgId, PolicyAttributeScope.ORGANISATION)); } + return attributesByOrgId; + } - for (ProducerDTO producer : producers) { - producer.setOrganisation(organisationFor(producer.getOrgId(), organisationsById, attributesByOrgId)); - for (ProductDTO product : producer.getProducts()) { - for (ConsumerDTO consumer : consumersOf(product)) { - consumer.setOrganisation( - organisationFor(consumer.getOrgId(), organisationsById, attributesByOrgId)); - } - } + /** + * One slot in the response graph that an {@link OrganisationDTO} has to be attached to: the + * organisation id to resolve (null when the owner has none) and where the resulting DTO goes. + * Lets the graph be walked once to collect ids and once to assign, without repeating the nested + * producer/product/consumer traversal. + */ + private record OrganisationHolder(Long orgId, Consumer setter) { + void assign(OrganisationDTO organisation) { + setter.accept(organisation); } } diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java index 471e88f..77185cd 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/providers/configuration/ConfigurationProviderImplTest.java @@ -678,4 +678,117 @@ void getConsumerConfigByClientId_neverCallsPolicyAttributeService() { verifyNoInteractions(policyAttributeService); } + + @Test + void getProducerConfigByClientId_producerWithNoOrgId_getsNullOrganisationWhileItsConsumersResolveTheirs() { + String clientId = "producerNoOrgClient"; + ProductDTO product = product(700L, "prod"); + // no orgId on the producer, unlike the producer(..) helper + ProducerDTO producer = ProducerDTO.builder() + .id(75L) + .active(true) + .idpClientId("cid") + .name("p") + .build(); + producer.getProducts().add(product); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + + ProductConsumerDTO subscription = productConsumer(700L, 706L, null, null); + subscription.setId(9006L); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of(subscription)); + + ConsumerDTO consumer = ConsumerDTO.builder().name("c706").orgId(801L).build(); + consumer.setId(706L); + when(consumerService.findById(706L)).thenReturn(Optional.of(consumer)); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 801L, + OrganisationDTO.builder() + .name("Consumer Org") + .key("CONS_ORG") + .build())); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + ProducerDTO returnedProducer = cfg.getProducers().get(0); + // the lookup does happen - a consumer needs it - but the producer has nothing to resolve + verify(organisationService).findByIds(Set.of(801L)); + assertThat(returnedProducer.getOrganisation()).isNull(); + assertThat(cfg.getOrganisation()).isNull(); + assertThat(returnedProducer.getProducts().get(0).getConsumers().get(0).getOrganisation()) + .isNotNull(); + assertThat(returnedProducer + .getProducts() + .get(0) + .getConsumers() + .get(0) + .getOrganisation() + .getKey()) + .isEqualTo("CONS_ORG"); + } + + @Test + void getProducerConfigByClientId_organisationSharedByTwoConsumers_isResolvedOnceButNotSharedByReference() { + String clientId = "sharedOrgClient"; + ProductDTO productA = product(700L, "prodA"); + ProductDTO productB = product(701L, "prodB"); + ProducerDTO producer = producer(76L, true, productA, productB); + when(producerService.getProducersByClientId(clientId)).thenReturn(List.of(producer)); + when(consumerService.getConsumersOfProviders(any())).thenReturn(Map.of()); + + ProductConsumerDTO subscriptionA = productConsumer(700L, 707L, null, null); + subscriptionA.setId(9007L); + ProductConsumerDTO subscriptionB = productConsumer(701L, 708L, null, null); + subscriptionB.setId(9008L); + when(productConsumerService.findByDataProviderId(700L)).thenReturn(List.of(subscriptionA)); + when(productConsumerService.findByDataProviderId(701L)).thenReturn(List.of(subscriptionB)); + + // two different consumers under two different products, both in organisation 801 + ConsumerDTO consumerA = ConsumerDTO.builder().name("c707").orgId(801L).build(); + consumerA.setId(707L); + ConsumerDTO consumerB = ConsumerDTO.builder().name("c708").orgId(801L).build(); + consumerB.setId(708L); + when(consumerService.findById(707L)).thenReturn(Optional.of(consumerA)); + when(consumerService.findById(708L)).thenReturn(Optional.of(consumerB)); + + PolicyAttributeDTO orgAttr = PolicyAttributeDTO.builder() + .name("classification") + .value("OFFICIAL") + .build(); + when(policyAttributeService.findAttributes(801L, PolicyAttributeScope.ORGANISATION)) + .thenReturn(List.of(orgAttr)); + when(organisationService.findByIds(any())) + .thenReturn(Map.of( + 1L, + OrganisationDTO.builder() + .name("Producer Org") + .key("PROD_ORG") + .build(), + 801L, + OrganisationDTO.builder() + .name("Consumer Org") + .key("CONS_ORG") + .build())); + + ProducerConfigDTO cfg = configurationProvider.getProducerConfigByClientId(clientId, Optional.empty()); + + // the duplicate org id is collapsed before the lookup, and resolved exactly once + verify(organisationService, times(1)).findByIds(Set.of(1L, 801L)); + verify(policyAttributeService, times(1)).findAttributes(801L, PolicyAttributeScope.ORGANISATION); + + ProducerDTO returnedProducer = cfg.getProducers().get(0); + OrganisationDTO orgOfA = + returnedProducer.getProducts().get(0).getConsumers().get(0).getOrganisation(); + OrganisationDTO orgOfB = + returnedProducer.getProducts().get(1).getConsumers().get(0).getOrganisation(); + + assertThat(orgOfA.getKey()).isEqualTo("CONS_ORG"); + assertThat(orgOfB.getKey()).isEqualTo("CONS_ORG"); + assertThat(orgOfA.getPolicyAttributes()).containsExactly(orgAttr); + assertThat(orgOfB.getPolicyAttributes()).containsExactly(orgAttr); + // each owner gets its own instance, so mutating one cannot leak into the other + assertThat(orgOfA).isNotSameAs(orgOfB); + assertThat(returnedProducer.getOrganisation()).isNotSameAs(orgOfA); + } } From 309688dc4fd0bd0152b5b8370fffd3ed064af300 Mon Sep 17 00:00:00 2001 From: nikan Negaresh Date: Fri, 11 Sep 2026 17:26:39 +0100 Subject: [PATCH 15/15] test(scope): improve attribute scope extraction logic and simplify service test verification - Refactored `PolicyAttributeScopeTest` to enhance extraction logic using method references for `getCode`. - Simplified mock verification in `ProductDiscoveryControllerTest` by consolidating method arguments. --- .../controller/v1/ProductDiscoveryControllerTest.java | 6 +----- .../management/service/data/PolicyAttributeScopeTest.java | 3 ++- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java index 8cc6405..6d2ebb5 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/controller/v1/ProductDiscoveryControllerTest.java @@ -135,11 +135,7 @@ void noAuthorisedProducts_returnsEmptyListNotErrorAndPassesCriteriaThrough() thr .andExpect(jsonPath("$.products").isEmpty()); verify(productDiscoveryService) - .discover( - eq(new PolicyRequester("client-1", "test-organisation", null)), - eq("Alpha"), - eq("topic-1"), - eq("TypeA")); + .discover(new PolicyRequester("client-1", "test-organisation", null), "Alpha", "topic-1", "TypeA"); } @Test diff --git a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java index 6959a2b..b78fa0e 100644 --- a/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java +++ b/src/test/java/uk/gov/dbt/ndtp/ia/node/management/service/data/PolicyAttributeScopeTest.java @@ -12,6 +12,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; import org.springframework.beans.factory.annotation.Autowired; +import uk.gov.dbt.ndtp.ia.node.management.persistency.entity.AttributeScope; import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AbstractPostgresRepositoryTest; import uk.gov.dbt.ndtp.ia.node.management.persistency.repository.AttributeScopeRepository; @@ -37,7 +38,7 @@ void coversEverySeededScopeCode() { .extracting(PolicyAttributeScope::code) .containsExactlyInAnyOrder("PRODUCER", "PRODUCT", "CONSUMER", "ORGANISATION", "SUBSCRIPTION"); assertThat(attributeScopeRepository.findAll()) - .extracting(scope -> scope.getCode()) + .extracting(AttributeScope::getCode) .containsExactlyInAnyOrderElementsOf(java.util.Arrays.stream(PolicyAttributeScope.values()) .map(PolicyAttributeScope::code) .toList());