diff --git a/api/src/org/labkey/api/data/DbScope.java b/api/src/org/labkey/api/data/DbScope.java index 022e7ef2870..5c8e1f2be51 100644 --- a/api/src/org/labkey/api/data/DbScope.java +++ b/api/src/org/labkey/api/data/DbScope.java @@ -591,7 +591,7 @@ private static LabKeyDataSource setPrimaryDataSource(Map driverClass) { try diff --git a/api/src/org/labkey/api/data/TableSelectorTestCase.java b/api/src/org/labkey/api/data/TableSelectorTestCase.java index 4d1969eea0b..bd1ded108c1 100644 --- a/api/src/org/labkey/api/data/TableSelectorTestCase.java +++ b/api/src/org/labkey/api/data/TableSelectorTestCase.java @@ -38,84 +38,92 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.stream.Stream; -/** -* User: adam -* Date: 1/19/12 -* Time: 5:54 PM -*/ public class TableSelectorTestCase extends AbstractSelectorTestCase { @Test public void testTableSelector() throws SQLException { -// Calls below can be used to test that Oracle and MySQL dialects behave as expected, following our maxRows, offset, -// and other rules. Uncomment these lines and their corresponding bean classes below. + +// Call below can be used to test that Oracle dialect behaves as expected, following our maxRows, offset, and other +// rules. Uncomment this line and the corresponding bean class below. // testTableSelector(DbSchema.get("oracle.granite", DbSchemaType.Bare).getTable("account"), Account.class); -// testTableSelector(DbSchema.get("mySql.sakila", DbSchemaType.Bare).getTable("country"), Country.class); + + // Test MySQL or MariaDB database, if present + List mySqlScopes = Stream.of("mySql", "mariadb") + .map(DbScope::getDbScope).filter(Objects::nonNull).toList(); + for (DbScope mySqlScope: mySqlScopes) + { + DbSchema sakila = mySqlScope.getSchema("sakila", DbSchemaType.Bare); + if (sakila.existsInDatabase()) + testTableSelector(sakila.getTable("country"), Country.class); + } testTableSelector(CoreSchema.getInstance().getTableInfoActiveUsers(), User.class); testTableSelector(CoreSchema.getInstance().getTableInfoModules(), ModuleContext.class); } -// public static class Country -// { -// private int _country_id; -// private String _country; -// private Date _last_update; -// -// public int getCountry_id() -// { -// return _country_id; -// } -// -// public void setCountry_id(int country_id) -// { -// _country_id = country_id; -// } -// -// public String getCountry() -// { -// return _country; -// } -// -// public void setCountry(String country) -// { -// _country = country; -// } -// -// public Date getLast_update() -// { -// return _last_update; -// } -// -// public void setLast_update(Date last_update) -// { -// _last_update = last_update; -// } -// -// @Override -// public boolean equals(Object o) -// { -// if (this == o) return true; -// if (o == null || getClass() != o.getClass()) return false; -// Country country = (Country) o; -// return _country_id == country._country_id && Objects.equals(_country, country._country) && Objects.equals(_last_update, country._last_update); -// } -// -// @Override -// public int hashCode() -// { -// return Objects.hash(_country_id, _country, _last_update); -// } -// } -// + @SuppressWarnings("unused") + public static class Country + { + private int _country_id; + private String _country; + private Date _last_update; + + public int getCountry_id() + { + return _country_id; + } + + public void setCountry_id(int country_id) + { + _country_id = country_id; + } + + public String getCountry() + { + return _country; + } + + public void setCountry(String country) + { + _country = country; + } + + public Date getLast_update() + { + return _last_update; + } + + public void setLast_update(Date last_update) + { + _last_update = last_update; + } + + @Override + public boolean equals(Object o) + { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Country country = (Country) o; + return _country_id == country._country_id && Objects.equals(_country, country._country) && Objects.equals(_last_update, country._last_update); + } + + @Override + public int hashCode() + { + return Objects.hash(_country_id, _country, _last_update); + } + } + // public static class Account // { // private int _account_id; diff --git a/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java b/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java index f2355b6bb7e..87e0d2f90d2 100644 --- a/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java +++ b/api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java @@ -36,7 +36,7 @@ public String getDatabase(String url) throws ServletException return parseDatabase(url.substring(_prefix.length())); } - protected String parseDatabase(String url) + protected String parseDatabase(String url) throws ServletException { // Database name ends with '?' or end of the URL int dbEnd = url.indexOf('?'); @@ -44,12 +44,11 @@ protected String parseDatabase(String url) if (-1 == dbEnd) dbEnd = url.length(); - // Database name starts after the last '/' or ':' - int slash = url.lastIndexOf('/', dbEnd); - int colon = url.lastIndexOf(':', dbEnd); + // Last '/' is the database delimiter, except for "jdbc:postgresql:database" + char dbDelimiter = url.contains("/") ? '/' : ':'; + int dbDelimiterIndex = url.lastIndexOf(dbDelimiter, dbEnd); - int dbDelimiter = Math.max(slash, colon); - - return url.substring(dbDelimiter + 1, dbEnd); + // dbDelimiterIndex == 1 means no database name, so return empty string in that case + return dbDelimiterIndex != 1 ? url.substring(dbDelimiterIndex + 1, dbEnd) : ""; } } diff --git a/api/src/org/labkey/api/data/dialect/sqlKeywords.txt b/api/src/org/labkey/api/data/dialect/sqlKeywords.txt index 82cfb83bfc6..59fc0a37dbb 100644 --- a/api/src/org/labkey/api/data/dialect/sqlKeywords.txt +++ b/api/src/org/labkey/api/data/dialect/sqlKeywords.txt @@ -1,7 +1,7 @@ # A list of SQL keyword candidates -- all known and potential future keywords across all databases. We use this for testing. # Sources include the old, hard-coded keyword lists in the dialects, the reserved word lists in the PostgreSQL, MySQL, and # SQL Server documentation, and the return values of DatabaseMetaData.getSqlKeywords() for every database we support. -# Note: build is required after altering this file. +# Note: a build is required after altering this file. a abort abs @@ -241,6 +241,7 @@ degree delay_key_write delayed delete +delete_domain_id delimiter delimiters dense_rank @@ -281,6 +282,7 @@ dlurlscheme dlurlserver dlvalue do +do_domain_ids document domain double @@ -432,6 +434,7 @@ identity_insert identitycol if ignore +ignore_domain_ids ignore_server_ids ilike immediate @@ -749,6 +752,7 @@ p pack_keys pad page +page_checksum parallel parameter parameter_mode @@ -759,6 +763,7 @@ parameter_specific_name parameter_specific_schema parameters parse_gcol_expr +parse_vcol_expr parser partial partition @@ -851,6 +856,7 @@ redo_buffer_size redofile redundant ref +ref_system_id reference references referencing diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 10bcad77681..23367d82ec5 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1484,6 +1484,7 @@ public TabDisplayMode getTabDisplayMode() public @NotNull Set> getUnitTests() { return Set.of( + AdminController.FileRootPermissionTestCase.class, ApiJsonWriter.TestCase.class, ClassLoaderTestCase.class, CopyFileRootPipelineJob.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 98df842b77c..8c5f3d30816 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -6302,6 +6302,7 @@ else if (form.hasSiteDefaultRoot()) { if (service.isFileRootDisabled(ctx.getContainer()) || !service.isUseDefaultRoot(ctx.getContainer())) { + throwIfUnauthorizedFileRootChange(ctx, service, form); service.setIsUseDefaultRoot(ctx.getContainer(), true); changed = true; shouldCopyMove = true; @@ -6446,22 +6447,62 @@ private static void initiateCopyFilesPipelineJobs(ViewContext ctx, @NotNull List private static void throwIfUnauthorizedFileRootChange(ViewContext ctx, FileContentService service, FileManagementForm form) { - // test permissions. only site admins are able to turn on a custom file root for a folder - // this is only relevant if the folder is either being switched to a custom file root, - // or if the file root is changed. - if (!service.isUseDefaultRoot(ctx.getContainer())) - { - Path fileRootPath = service.getFileRootPath(ctx.getContainer()); - if (null != fileRootPath) + // Only site admins (AdminOperationsPermission) are able to switch a folder to a custom file root, change + // an existing custom root's path, or revert a custom root back to the site default -- any of these moves + // where the folder's files live. Resubmitting the folder's own current root unchanged is a no-op and does + // not require the elevated permission. + boolean hasAdminOpsPermission = ctx.getUser().hasRootPermission(AdminOperationsPermission.class); + boolean isUseDefaultRoot = service.isUseDefaultRoot(ctx.getContainer()); + String requestedRoot; + String currentRoot; + + if (form.hasSiteDefaultRoot()) + { + // Requesting the default root: no root is being submitted; the current custom root (if any) is + // whatever type the container has now. + requestedRoot = null; + if (isUseDefaultRoot) + currentRoot = null; + else if (service.isCloudRoot(ctx.getContainer())) + currentRoot = service.getCloudRootName(ctx.getContainer()); + else { - String absolutePath = FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath); - if (Strings.CI.equals(absolutePath, form.getFolderRootPath())) - { - if (!ctx.getUser().hasRootPermission(AdminOperationsPermission.class)) - throw new UnauthorizedException("Only site admins can change file roots"); - } + Path fileRootPath = service.getFileRootPath(ctx.getContainer()); + currentRoot = null != fileRootPath ? FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath) : null; } } + else if (form.isCloudFileRoot()) + { + requestedRoot = form.getCloudRootName(); + currentRoot = (!isUseDefaultRoot && service.isCloudRoot(ctx.getContainer())) ? service.getCloudRootName(ctx.getContainer()) : null; + } + else + { + requestedRoot = StringUtils.trimToNull(form.getFolderRootPath()); + Path fileRootPath = isUseDefaultRoot ? null : service.getFileRootPath(ctx.getContainer()); + currentRoot = null != fileRootPath ? FileUtil.getAbsolutePath(ctx.getContainer(), fileRootPath) : null; + } + + if (!isFileRootChangeAuthorizedOrNoChange(hasAdminOpsPermission, isUseDefaultRoot, currentRoot, requestedRoot)) + throw new UnauthorizedException("Only site admins can change file roots"); + } + + /** + * Pure decision logic behind {@link #throwIfUnauthorizedFileRootChange}, factored out for unit testing. + * @param hasAdminOpsPermission whether the requesting user holds root AdminOperationsPermission + * @param isUseDefaultRoot whether the target container currently uses the default (inherited) file root + * @param currentRoot the container's existing custom root path/cloud name, or null if there isn't one + * @param requestedRoot the root path/cloud name submitted in the request, or null if none was submitted + */ + private static boolean isFileRootChangeAuthorizedOrNoChange(boolean hasAdminOpsPermission, boolean isUseDefaultRoot, @Nullable String currentRoot, @Nullable String requestedRoot) + { + if (hasAdminOpsPermission) + return true; + if (null == requestedRoot) + return isUseDefaultRoot || null == currentRoot; // clearing an existing custom root is still a change to it + if (!isUseDefaultRoot && requestedRoot.equalsIgnoreCase(currentRoot)) + return true; // no-op resubmission of the folder's own existing custom root + return false; } public static void setEnabledCloudStores(ViewContext ctx, FileManagementForm form, BindException errors) @@ -9293,6 +9334,68 @@ public void modulesWithSchemaVersionButNoScripts() } } + // Regression coverage for the file root privilege-escalation fix: a folder/project admin without root + // AdminOperationsPermission must never be able to switch a container to a custom file root, or change an + // existing custom root's path. + public static class FileRootPermissionTestCase extends Assert + { + @Test + public void defaultRootRequiresAdminOpsPermissionForNewCustomPath() + { + // This is the case the original (inverted) condition silently skipped: a container on the default + // root, submitting any custom path, from a user without AdminOperationsPermission. + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, true, null, "/some/path")); + } + + @Test + public void customRootRequiresAdminOpsPermissionForDifferentPath() + { + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", "/attacker/path")); + } + + @Test + public void customRootResubmissionOfSamePathIsAllowed() + { + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", "/existing/path")); + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, false, "/Existing/Path", "/existing/path")); + } + + @Test + public void noRequestedRootOnDefaultRootIsAllowed() + { + // No custom root requested and none currently exists -- nothing to protect. + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, true, null, null)); + } + + @Test + public void clearingAnExistingCustomRootRequiresAdminOpsPermission() + { + // A request that omits the root (e.g. a non-ops admin's disabled form fields not being submitted) + // must not be able to silently clear an existing custom root back to default. + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", null)); + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, false, "/existing/path", null)); + } + + @Test + public void revertingCustomRootToSiteDefaultRequiresAdminOpsPermission() + { + // Selecting the site default root while a custom root (file path or cloud) is in effect relocates the + // folder's file storage, so it needs the same elevated permission as setting a custom root. + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "/existing/path", null)); + assertFalse(isFileRootChangeAuthorizedOrNoChange(false, false, "myCloudStore", null)); + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, false, "myCloudStore", null)); + // Already on the default root (e.g. re-enabling file sharing from the disabled state) is a no-op. + assertTrue(isFileRootChangeAuthorizedOrNoChange(false, true, null, null)); + } + + @Test + public void adminOpsPermissionIsAlwaysAllowed() + { + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, true, null, "/some/path")); + assertTrue(isFileRootChangeAuthorizedOrNoChange(true, false, "/existing/path", "/attacker/path")); + } + } + public static class ModuleForm { private String _name; diff --git a/core/src/org/labkey/core/security/addUsers.jsp b/core/src/org/labkey/core/security/addUsers.jsp index 80dcd6df68f..25741c0362f 100644 --- a/core/src/org/labkey/core/security/addUsers.jsp +++ b/core/src/org/labkey/core/security/addUsers.jsp @@ -40,11 +40,6 @@ boolean excludeSiteAdmins = !getUser().hasSiteAdminPermission(); // App admins can't clone permissions from site admins %>