From b40a3d2dc849790c351e7077cda3faf2a24ce97a Mon Sep 17 00:00:00 2001 From: Julian Ladisch Date: Wed, 21 Feb 2024 01:18:59 +0100 Subject: [PATCH] OKAPI-1081: Reject invalid tenant ids https://folio-org.atlassian.net/browse/OKAPI-1081 Implement https://folio-org.atlassian.net/wiki/display/TC/ADR-000002+-+Tenant+Id+and+Module+Name+Restrictions so that creating a new tenant via POST /_/proxy/tenants API is rejected unless the tenant id matches ^[a-z][a-z0-9]{0,30}$ Legacy tenant ids still work when used in any other API. --- .../folio/okapi/managers/InternalModule.java | 22 +++---- .../org/folio/okapi/util/TenantValidator.java | 58 +++++++++++++++++++ .../src/main/raml/TenantDescriptor.json | 2 +- .../infra-messages/Messages_en.properties | 7 ++- .../test/java/org/folio/okapi/ProxyTest.java | 8 +-- .../okapi/managers/InternalModuleTest.java | 11 ++++ .../folio/okapi/util/TenantValidatorTest.java | 36 ++++++++++++ 7 files changed, 126 insertions(+), 18 deletions(-) create mode 100644 okapi-core/src/main/java/org/folio/okapi/util/TenantValidator.java create mode 100644 okapi-core/src/test/java/org/folio/okapi/util/TenantValidatorTest.java diff --git a/okapi-core/src/main/java/org/folio/okapi/managers/InternalModule.java b/okapi-core/src/main/java/org/folio/okapi/managers/InternalModule.java index 094a93988..d9bbaf5b3 100644 --- a/okapi-core/src/main/java/org/folio/okapi/managers/InternalModule.java +++ b/okapi-core/src/main/java/org/folio/okapi/managers/InternalModule.java @@ -38,6 +38,7 @@ import org.folio.okapi.util.OkapiError; import org.folio.okapi.util.ProxyContext; import org.folio.okapi.util.TenantInstallOptions; +import org.folio.okapi.util.TenantValidator; /** * Okapi's built-in module. Managing /_/ endpoints. @@ -750,22 +751,21 @@ private String location(ProxyContext pc, String id, String baseUri, String s) { private Future createTenant(ProxyContext pc, String body) { try { final TenantDescriptor td = JsonDecoder.decode(body, TenantDescriptor.class); - if (td.getId() == null || td.getId().isEmpty()) { - td.setId(UUID.randomUUID().toString()); - } - final String tenantId = td.getId(); - if (!tenantId.matches("^[a-z0-9_-]+$")) { - return Future.failedFuture( - new OkapiError(ErrorType.USER, messages.getMessage("11601", tenantId))); - } - Tenant t = new Tenant(td); - return tenantManager.insert(t).map(res -> - location(pc, tenantId, null, Json.encodePrettily(t.getDescriptor()))); + return validateTenantId(td) + .compose(x -> tenantManager.insert(new Tenant(td))) + .map(x -> location(pc, td.getId(), null, Json.encodePrettily(td))); } catch (DecodeException ex) { return Future.failedFuture(new OkapiError(ErrorType.USER, ex.getMessage())); } } + static Future validateTenantId(TenantDescriptor td) { + if (td.getId() == null || td.getId().isEmpty()) { + td.setId("t" + UUID.randomUUID().toString().replace("-", "").substring(2)); + } + return TenantValidator.validate(td.getId()); + } + private Future updateTenant(String tenantId, String body) { try { final TenantDescriptor td = JsonDecoder.decode(body, TenantDescriptor.class); diff --git a/okapi-core/src/main/java/org/folio/okapi/util/TenantValidator.java b/okapi-core/src/main/java/org/folio/okapi/util/TenantValidator.java new file mode 100644 index 000000000..6e237b631 --- /dev/null +++ b/okapi-core/src/main/java/org/folio/okapi/util/TenantValidator.java @@ -0,0 +1,58 @@ +package org.folio.okapi.util; + +import io.vertx.core.Future; +import java.util.regex.Pattern; +import org.folio.okapi.common.ErrorType; +import org.folio.okapi.common.Messages; + +/** + * Validate a tenant ID to match ^[a-z][a-z0-9]{0,30}$ as required by + * + * https://folio-org.atlassian.net/wiki/display/TC/ADR-000002+-+Tenant+Id+and+Module+Name+Restrictions + * Technical Council decision. + */ +public final class TenantValidator { + + // multi-byte sequences forbidden in pattern, so char length = byte length + private static final String TENANT_PATTERN_STRING = "^[a-z][a-z0-9]{0,30}$"; + private static final Pattern TENANT_PATTERN = Pattern.compile(TENANT_PATTERN_STRING); + private static final Pattern STARTS_WITH_DIGIT = Pattern.compile("^[0-9]"); + private static final Messages MESSAGES = Messages.getInstance(); + + private TenantValidator() { + throw new UnsupportedOperationException("Cannot instantiate utility class."); + } + + /** + * Validate tenantId against ^[a-z][a-z0-9]{0,30}$ and return a failed Future with + * clear violation explanation message on validation failure. + */ + public static Future validate(String tenantId) { + if (TENANT_PATTERN.matcher(tenantId).matches()) { + return Future.succeededFuture(); + } + + if (tenantId.contains("_")) { + return failure("11609", tenantId); + } + + if (tenantId.contains("-")) { + return failure("11610", tenantId); + } + + if (tenantId.length() > 31) { + return failure("11611", tenantId); + } + + if (STARTS_WITH_DIGIT.matcher(tenantId).matches()) { + return failure("11612", tenantId); + } + + return failure("11601", tenantId); + } + + private static Future failure(String errorMessage, String tenantId) { + return Future.failedFuture(new OkapiError(ErrorType.USER, + MESSAGES.getMessage(errorMessage, tenantId))); + } +} diff --git a/okapi-core/src/main/raml/TenantDescriptor.json b/okapi-core/src/main/raml/TenantDescriptor.json index 08dcd0c06..bc3b09080 100644 --- a/okapi-core/src/main/raml/TenantDescriptor.json +++ b/okapi-core/src/main/raml/TenantDescriptor.json @@ -7,7 +7,7 @@ "additionalProperties" : false, "properties": { "id": { - "description": "Tenant ID", + "description": "Tenant ID. A new tenant ID created via the POST /_/proxy/tenants API must match the ^[a-z][a-z0-9]{0,30}$ regexp as required by https://folio-org.atlassian.net/wiki/spaces/TC/pages/5053983/DR-000002+-+Tenant+Id+and+Module+Name+Restrictions Technical Council decision. All other APIs also accept a legacy tenant ID that matches ^[a-z0-9_-]+$ regexp and may pose security issues as explained on https://folio-org.atlassian.net/wiki/spaces/DD/pages/1779867/Tenant+Id+and+Module+Name+Restrictions", "type": "string" }, "name": { diff --git a/okapi-core/src/main/resources/infra-messages/Messages_en.properties b/okapi-core/src/main/resources/infra-messages/Messages_en.properties index 4829b12db..9fe83bb63 100644 --- a/okapi-core/src/main/resources/infra-messages/Messages_en.properties +++ b/okapi-core/src/main/resources/infra-messages/Messages_en.properties @@ -107,7 +107,7 @@ #InternalModule 11600=Error in encoding location id {0}. {1} -11601=Invalid tenant id {0} +11601=Invalid tenant id, may only contain a-z and 0-9 and must match '[a-z][a-z0-9]{0,30}' but it is {0} 11602=Tenant.id={0} id={1} 11603=Can not delete the superTenant {0} 11604=unknown orderBy field: {0} @@ -115,4 +115,7 @@ 11606=Module.id={0} id={1} 11607=Unhandled internal module path={0} 11608=Bad format for parameter {0}. {1} - +11609=Tenant id must not contain underscore: {0} +11610=Tenant id must not contain minus: {0} +11611=Tenant id must not exceed 31 characters: {0} +11612=Tenant id must not start with a digit: {0} diff --git a/okapi-core/src/test/java/org/folio/okapi/ProxyTest.java b/okapi-core/src/test/java/org/folio/okapi/ProxyTest.java index 454551c9b..68338258a 100644 --- a/okapi-core/src/test/java/org/folio/okapi/ProxyTest.java +++ b/okapi-core/src/test/java/org/folio/okapi/ProxyTest.java @@ -3377,7 +3377,7 @@ public void testTenantFailedUpgrade(TestContext context) { @Test public void testProxyClientFailure(TestContext context) { - String tenant = "test-tenant-permissions-tenant"; + String tenant = "testtenantpermissionstenant"; setupBasicTenant(tenant); String moduleId = "module-1.0.0"; @@ -3415,7 +3415,7 @@ public void testProxyClientFailure(TestContext context) { @Test public void testTenantPermissionsUpgrade() { - String tenant = "test-tenant-permissions-tenant"; + String tenant = "testtenantpermissionstenant"; setupBasicTenant(tenant); String moduleA0 = "moduleA-1.0.0"; @@ -3465,7 +3465,7 @@ private boolean hasPermReplaces(JsonArray permissions) { @Test public void testTenantPermissionsVersion() { - String tenant = "test-tenant-permissions-tenant"; + String tenant = "testtenantpermissionstenant"; String moduleId = "test-tenant-permissions-basic-module-1.0.0"; String authModuleId = "test-tenant-permissions-auth-module-1.0.0"; String body = new JsonObject().put("id", "test").encode(); @@ -3522,7 +3522,7 @@ public void testTenantPermissionsVersion() { @Test public void testDelegateCORS() { - String tenant = "test-tenant-delegate-cors"; + String tenant = "testtenantdelegatecors"; String moduleId = "test-tenant-delegate-cors-module-1.0.0"; String authModuleId = "test-tenant-delegate-cors-auth-module-1.0.0"; String body = new JsonObject().put("id", "test").encode(); diff --git a/okapi-core/src/test/java/org/folio/okapi/managers/InternalModuleTest.java b/okapi-core/src/test/java/org/folio/okapi/managers/InternalModuleTest.java index 143f3202b..f6d8049ab 100644 --- a/okapi-core/src/test/java/org/folio/okapi/managers/InternalModuleTest.java +++ b/okapi-core/src/test/java/org/folio/okapi/managers/InternalModuleTest.java @@ -8,11 +8,13 @@ import io.vertx.junit5.VertxExtension; import io.vertx.junit5.VertxTestContext; import org.assertj.core.api.WithAssertions; +import org.folio.okapi.bean.TenantDescriptor; import org.folio.okapi.util.ProxyContext; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.NullAndEmptySource; import org.mockito.junit.jupiter.MockitoExtension; @Timeout(5) @@ -104,4 +106,13 @@ void invalidJson(HttpMethod httpMethod, String path, VertxTestContext vtc) { vtc.completeNow(); })); } + + @ParameterizedTest + @NullAndEmptySource + void validateTenantId(String tenantId) { + var td = new TenantDescriptor(tenantId, "foo"); + assertThat(InternalModule.validateTenantId(td).succeeded()).isTrue(); + assertThat(td.getId()).matches("t[0-9a-f]{30}"); + assertThat(td.getName()).isEqualTo("foo"); + } } diff --git a/okapi-core/src/test/java/org/folio/okapi/util/TenantValidatorTest.java b/okapi-core/src/test/java/org/folio/okapi/util/TenantValidatorTest.java new file mode 100644 index 000000000..fcdc839cc --- /dev/null +++ b/okapi-core/src/test/java/org/folio/okapi/util/TenantValidatorTest.java @@ -0,0 +1,36 @@ +package org.folio.okapi.util; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.folio.okapi.testing.UtilityClassTester; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +class TenantValidatorTest { + + @Test + void utilityClass() { + UtilityClassTester.assertUtilityClass(TenantValidator.class); + } + + @ParameterizedTest + @CsvSource({ + "a, ", + "a1, ", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, ", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, Tenant id must not exceed 31 characters: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "1, Tenant id must not start with a digit: 1", + "foo_bar, Tenant id must not contain underscore: foo_bar", + "a-z, Tenant id must not contain minus: a-z", + "universität, 'Invalid tenant id, may only contain a-z and 0-9 and must match [a-z][a-z0-9]{0,30} but it is universität'", + }) + void validate(String tenantId, String errorMessage) { + var result = TenantValidator.validate(tenantId); + if (errorMessage == null) { + assertThat(result.succeeded()).isTrue(); + } else { + assertThat(result.cause().getMessage()).isEqualTo(errorMessage); + } + } +}