Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

Merged
merged 1 commit into from
Sep 27, 2024
Merged

MODFQMMGR-456: Check whether entity type is cross-tenant when retrieving definition #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions src/main/java/org/folio/fqm/service/CrossTenantQueryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@ public class CrossTenantQueryService {
private final SimpleHttpClient ecsClient;
private final FolioExecutionContext executionContext;
private final PermissionsService permissionsService;
private final UserTenantService userTenantService;

private static final String COMPOSITE_INSTANCES_ID = "6b08439b-4f8e-4468-8046-ea620f5cfb74";
private static final String SIMPLE_INSTANCES_ID = "8fc4a9d2-7ccf-4233-afb8-796911839862";

public List<String> getTenantsToQuery(EntityType entityType, boolean forceCrossTenantQuery) {
if (!forceCrossTenantQuery && !Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())) {
if (!forceCrossTenantQuery
&& !Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())
&& !COMPOSITE_INSTANCES_ID.equals(entityType.getId())) {
Comment on lines -30 to +33
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate how complicated this condition is getting, but we do need all these checks. The new check for the composite instances ID is due to the fact that cross-tenant queries will be disabled for non-central tenants, but we DO still need to run a (pseudo) cross tenant query for composite instances in member tenants in order to get the shared instances from the central tenant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to run into issues with the tenant ID dropdown menu for instances, since the entity type that gets passed in is simple_instances, which doesn't have cross-tenant queries enabled :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up needing to check both composite and simple instances in 2 places in this method, I think I'd make a private static final Set<String> to hold the IDs, then do a contains(entityType.getId()) instead of checking both IDs separately

Copy link
Collaborator Author

@bvsharp bvsharp Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This situation actually gets handled by the forceCrossTenantQuery parameter! When we get the tenant ID values, we pass forceCrossTenantQuery=true to this method. Since it's true, we skip this block. From there:

  1. If ECS is not enabled, we return only the current tenant.
  2. If ECS is enabled but we're not in the central tenant AND the entity type is composite/simple instances, we return the current tenant plus the central tenant (in order to get the shared instances).
  3. If ECS is enabled and we're in the central tenant, then we query everything we have permission for.

So I think everything related to the tenant ID dropdown is handled. The only thing that the current code doesn't handle is running a (non-tenant ID related) query on the simple_instances entity from a member tenant. In that situation, we'd only be querying the member tenant, so we wouldn't see the shared records from the central tenant (although this is counter-balanced by the fact that simple_instances doesn't have an additionalEcsConditions defined either, meaning that we wouldn't be filtering out shadow instances. So in this situation we'd see ALMOST the same thing as for composite_instances, only we'd be seeing the shadow instances instead of the shared ones).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I don't think we'd want to include simple_instances in the first check (at least the way we have the definitions set up right now), due to the fact the simple_instances doesn't filter out the shadow instances. So if we added the simple_instance check to the first block, we'd end up with duplicate instance records in the query results (one shadow version and one shared version for each shared instance).

return List.of(executionContext.getTenantId());
}
// Get the ECS tenant info first, since this comes from mod-users and should work in non-ECS environments
Expand Down Expand Up @@ -82,6 +85,23 @@ private List<Map<String, String>> getUserTenants(String consortiumId, String use
.parse(userTenantResponse)
.read("$.userTenants", List.class);
}

public String getCentralTenantId() {
return getCentralTenantId(getEcsTenantInfo());
}

public boolean ecsEnabled() {
return ecsEnabled(getEcsTenantInfo());
}

public boolean isCentralTenant() {
return isCentralTenant(getEcsTenantInfo());
}

private boolean ecsEnabled(Map<String, String> ecsTenantInfo) {
return !(ecsTenantInfo == null || ecsTenantInfo.isEmpty());
}

/**
* Retrieve the primary affiliation for a user.
* This retrieves the primary affiliation for an arbitrary user in the tenant.
Expand All @@ -90,7 +110,7 @@ private List<Map<String, String>> getUserTenants(String consortiumId, String use
*/
@SuppressWarnings("unchecked") // JsonPath.parse is returning a plain List without a type parameter, and the TypeRef (vs Class) parameter to JsonPath.read is not supported by the JSON parser
private Map<String, String> getEcsTenantInfo() {
String userTenantsResponse = ecsClient.get("user-tenants", Map.of("limit", "1"));
String userTenantsResponse = userTenantService.getUserTenantsResponse(executionContext.getTenantId());
List<Map<String, String>> userTenants = JsonPath
.parse(userTenantsResponse)
.read("$.userTenants", List.class);
Expand All @@ -104,15 +124,7 @@ private String getCentralTenantId(Map<String, String> ecsTenantInfo) {
return ecsTenantInfo != null ? ecsTenantInfo.get("centralTenantId") : null;
}

public String getCentralTenantId() {
return getCentralTenantId(getEcsTenantInfo());
}

private boolean ecsEnabled(Map<String, String> ecsTenantInfo) {
return !(ecsTenantInfo == null || ecsTenantInfo.isEmpty());
}

public boolean ecsEnabled() {
return ecsEnabled(getEcsTenantInfo());
private boolean isCentralTenant(Map<String, String> ecsTenantInfo) {
return executionContext.getTenantId().equals(getCentralTenantId(ecsTenantInfo));
}
}
9 changes: 5 additions & 4 deletions src/main/java/org/folio/fqm/service/EntityTypeFlatteningService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import com.jayway.jsonpath.JsonPath;
import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import org.folio.fqm.client.SimpleHttpClient;
import org.folio.fqm.exception.EntityTypeNotFoundException;
import org.folio.fqm.exception.InvalidEntityTypeDefinitionException;
import org.folio.fqm.repository.EntityTypeRepository;
Expand All @@ -17,6 +16,7 @@
import org.folio.querytool.domain.dto.Field;
import org.folio.querytool.domain.dto.NestedObjectProperty;
import org.folio.querytool.domain.dto.ObjectType;
import org.folio.spring.FolioExecutionContext;
import org.springframework.stereotype.Service;

import java.util.*;
Expand All @@ -31,7 +31,8 @@ public class EntityTypeFlatteningService {
private final EntityTypeRepository entityTypeRepository;
private final ObjectMapper objectMapper;
private final LocalizationService localizationService;
private final SimpleHttpClient ecsClient;
private final FolioExecutionContext executionContext;
private final UserTenantService userTenantService;

public EntityType getFlattenedEntityType(UUID entityTypeId, String tenantId) {
return getFlattenedEntityType(entityTypeId, null, tenantId);
Expand Down Expand Up @@ -317,8 +318,8 @@ private Stream<EntityTypeColumn> getFilteredColumns(Stream<EntityTypeColumn> unf
}

private boolean ecsEnabled() {
String rawJson = ecsClient.get("user-tenants", Map.of("limit", String.valueOf(1)));
DocumentContext parsedJson = JsonPath.parse(rawJson);
String userTenantsResponse = userTenantService.getUserTenantsResponse(executionContext.getTenantId());
DocumentContext parsedJson = JsonPath.parse(userTenantsResponse);
// The value isn't needed here, this just provides an easy way to tell if ECS is enabled
int totalRecords = parsedJson.read("totalRecords", Integer.class);
return totalRecords > 0;
Expand Down
9 changes: 7 additions & 2 deletions src/main/java/org/folio/fqm/service/EntityTypeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ public List<EntityTypeSummary> getEntityTypeSummary(Set<UUID> entityTypeIds, boo
.map(entityType -> {
EntityTypeSummary result = new EntityTypeSummary()
.id(UUID.fromString(entityType.getId()))
.label(localizationService.getEntityTypeLabel(entityType.getName()));
.label(localizationService.getEntityTypeLabel(entityType.getName()))
.crossTenantQueriesEnabled(entityType.getCrossTenantQueriesEnabled());
if (includeInaccessible) {
return result.missingPermissions(
permissionsService.getRequiredPermissions(entityType)
Expand All @@ -86,6 +87,8 @@ public List<EntityTypeSummary> getEntityTypeSummary(Set<UUID> entityTypeIds, boo
*/
public EntityType getEntityTypeDefinition(UUID entityTypeId, boolean includeHidden, boolean sortColumns) {
EntityType entityType = entityTypeFlatteningService.getFlattenedEntityType(entityTypeId, null);
boolean crossTenantEnabled = Boolean.TRUE.equals(entityType.getCrossTenantQueriesEnabled())
&& crossTenantQueryService.isCentralTenant();
Comment on lines +90 to +91
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a weird way to do it, but this essentially overwrites the crossTenantQueriesEnabled property, so that non-central tenants have it set to false, even if it's true in the original definition. This will allow us to avoid forcing lists for cross-tenant entity types to be private in member tenants, and instead only apply that effect to lists in the central tenant. Still not crazy about this approach but it seems to be the simplest option for now.

List<EntityTypeColumn> columns = entityType
.getColumns()
.stream()
Expand All @@ -96,7 +99,9 @@ public EntityType getEntityTypeDefinition(UUID entityTypeId, boolean includeHidd
.sorted(nullsLast(comparing(Field::getLabelAlias, String.CASE_INSENSITIVE_ORDER)))
.toList();
}
return entityType.columns(columns);
return entityType
.columns(columns)
.crossTenantQueriesEnabled(crossTenantEnabled);
}

/**
Expand Down
26 changes: 26 additions & 0 deletions src/main/java/org/folio/fqm/service/UserTenantService.java
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on breaking this out into a separate service!

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package org.folio.fqm.service;

import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import org.folio.fqm.client.SimpleHttpClient;
import org.springframework.cache.annotation.Cacheable;
import org.springframework.stereotype.Service;

import java.util.Map;

/**
* Service wrapper for caching responses from user-tenants API.
*/
@Service
@RequiredArgsConstructor
@Log4j2
public class UserTenantService {

private final SimpleHttpClient userTenantsClient;

@Cacheable(value="userTenantCache", key="#tenantId")
public String getUserTenantsResponse(String tenantId) {
log.info("Retrieving user-tenants information for tenant {}", tenantId);
return userTenantsClient.get("user-tenants", Map.of("limit", String.valueOf(1)));
}
}
Comment on lines +1 to +26
Copy link
Collaborator Author

@bvsharp bvsharp Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this class to allow for convenient caching for the user-tenants responses. We use the /user-tenants endpoint in both the EntityTypeFlatteningService and the CrossTenantQueryService, so I figured adding a service wrapper made the most sense to allow us to:

  1. Cache the user-tenants responses in a convenient way regardless of what class the request is made from, and without code duplication.
  2. Avoid caching responses on any of the other SimpleHttpClient requests.

1 change: 1 addition & 0 deletions src/main/resources/application.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ coffee-boots:
cache:
spec:
queryCache: maximumSize=500,expireAfterWrite=1m
userTenantCache: maximumSize=100,expireAfterWrite=5h
Copy link
Collaborator Author

@bvsharp bvsharp Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picked a fairly arbitrary expire time, and am open to any other values for this cache. Also open to making it a configurable value, though I didn't do make it configurable for the time being, since I'm not sure there's any value in doing so.

folio:
is-eureka: false
tenant:
Expand Down
5 changes: 5 additions & 0 deletions src/main/resources/swagger.api/schemas/EntityTypeSummary.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
"description": "Entity type label",
"type": "string"
},
"crossTenantQueriesEnabled": {
"description": "Indicates if this entity type supports cross-tenant queries",
"type": "boolean",
"default": false
},
"missingPermissions": {
"description": "List of missing permissions",
"type": "array",
Expand Down
60 changes: 46 additions & 14 deletions src/test/java/org/folio/fqm/service/CrossTenantQueryServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ class CrossTenantQueryServiceTest {
@Mock
private PermissionsService permissionsService;

@Mock
private UserTenantService userTenantService;

@InjectMocks
private CrossTenantQueryService crossTenantQueryService;

Expand All @@ -59,17 +62,20 @@ class CrossTenantQueryServiceTest {
{
"id": "06192681-0df7-4f33-a38f-48e017648d69",
"userId": "a5e7895f-503c-4335-8828-f507bc8d1c45",
"tenantId": "tenant_01"
"tenantId": "tenant_01",
"centralTenantId": "tenant_01"
},
{
"id": "3c1bfbe9-7d64-41fe-a358-cdaced6a631f",
"userId": "a5e7895f-503c-4335-8828-f507bc8d1c45",
"tenantId": "tenant_02"
"tenantId": "tenant_02",
"centralTenantId": "tenant_01"
},
{
"id": "b167837a-ecdd-482b-b5d3-79a391a1dbf1",
"userId": "a5e7895f-503c-4335-8828-f507bc8d1c45",
"tenantId": "tenant_03",
"centralTenantId": "tenant_01"
}
]
}
Expand All @@ -93,7 +99,7 @@ void shouldGetListOfTenantsToQuery() {
List<String> expectedTenants = List.of("tenant_01", "tenant_02", "tenant_03");

when(executionContext.getTenantId()).thenReturn(tenantId);
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO);
when(ecsClient.get(eq("consortia/bdaa4720-5e11-4632-bc10-d4455cf252df/user-tenants"), anyMap())).thenReturn(USER_TENANT_JSON);

List<String> actualTenants = crossTenantQueryService.getTenantsToQuery(entityType, false);
Expand All @@ -112,18 +118,20 @@ void shouldRunIntraTenantQueryForNonInstanceEntityTypes() {

@Test
void shouldRunIntraTenantQueryForNonCentralTenant() {
List<String> expectedTenants = List.of("tenant_02");
when(executionContext.getTenantId()).thenReturn("tenant_02"); // Central is tenant_01
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO);
String tenantId = "tenant_02";
List<String> expectedTenants = List.of(tenantId);
when(executionContext.getTenantId()).thenReturn(tenantId); // Central is tenant_01
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO);
List<String> actualTenants = crossTenantQueryService.getTenantsToQuery(entityType, false);
assertEquals(expectedTenants, actualTenants);
}

@Test
void shouldRunIntraTenantQueryIfExceptionIsThrown() {
List<String> expectedTenants = List.of("tenant_01");
when(executionContext.getTenantId()).thenReturn("tenant_01");
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);
String tenantId = "tenant_01";
List<String> expectedTenants = List.of(tenantId);
when(executionContext.getTenantId()).thenReturn(tenantId);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);
List<String> actualTenants = crossTenantQueryService.getTenantsToQuery(entityType, false);
assertEquals(expectedTenants, actualTenants);
}
Expand All @@ -134,19 +142,32 @@ void shouldReturnTenantIdOnlyIfUserTenantsApiThrowsException() {
List<String> expectedTenants = List.of("tenant_01");

when(executionContext.getTenantId()).thenReturn(tenantId);
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);

List<String> actualTenants = crossTenantQueryService.getTenantsToQuery(entityType, false);
assertEquals(expectedTenants, actualTenants);
}

@Test
void shouldAttemptCrossTenantQueryIfForceParamIsTrue() {
String tenantId = "tenant_01";
List<String> expectedTenants = List.of("tenant_01");

when(executionContext.getTenantId()).thenReturn(tenantId);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);

List<String> actualTenants = crossTenantQueryService.getTenantsToQuery(entityType, true);
verify(userTenantService, times(1)).getUserTenantsResponse(tenantId);
assertEquals(expectedTenants, actualTenants);
}

@Test
void shouldNotQueryTenantIfUserLacksTenantPermissions() {
String tenantId = "tenant_01";
List<String> expectedTenants = List.of("tenant_01", "tenant_02");

when(executionContext.getTenantId()).thenReturn(tenantId);
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO);
when(ecsClient.get(eq("consortia/bdaa4720-5e11-4632-bc10-d4455cf252df/user-tenants"), anyMap())).thenReturn(USER_TENANT_JSON);
doNothing().when(permissionsService).verifyUserHasNecessaryPermissions("tenant_02", entityType, true);
doThrow(MissingPermissionsException.class).when(permissionsService).verifyUserHasNecessaryPermissions("tenant_03", entityType, true);
Expand All @@ -163,23 +184,34 @@ void shouldQueryCentralTenantForSharedCompositeInstances() {
.crossTenantQueriesEnabled(true);

when(executionContext.getTenantId()).thenReturn(tenantId);
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO);

List<String> actualTenants = crossTenantQueryService.getTenantsToQuery(instanceEntityType, false);
assertEquals(expectedTenants, actualTenants);
}

@Test
void shouldGetCentralTenantId() {
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO);
String expectedId = "tenant_01";
when(executionContext.getTenantId()).thenReturn(expectedId);
when(userTenantService.getUserTenantsResponse(expectedId)).thenReturn(ECS_TENANT_INFO);
String actualId = crossTenantQueryService.getCentralTenantId();
assertEquals(expectedId, actualId);
}

@Test
void shouldHandleErrorWhenGettingCentralTenantId() {
when(ecsClient.get(eq("user-tenants"), anyMap())).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);
String tenantId = "tenant_01";
when(executionContext.getTenantId()).thenReturn(tenantId);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(ECS_TENANT_INFO_FOR_NON_ECS_ENV);
assertNull(crossTenantQueryService.getCentralTenantId());
}

@Test
void testIsCentralTenant() {
String tenantId = "tenant_01";
when(executionContext.getTenantId()).thenReturn(tenantId);
when(userTenantService.getUserTenantsResponse(tenantId)).thenReturn(USER_TENANT_JSON);
assertTrue(crossTenantQueryService.isCentralTenant());
}
}
Loading
Loading