[bot] Merge 26.8 to develop

api/src/org/labkey/api/data/TableSelectorTestCase.java

@@ -17,6 +17,7 @@
 
 import org.apache.commons.lang3.mutable.MutableInt;
 import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Logger;
 import org.junit.Test;
 import org.labkey.api.collections.CsvSet;
 import org.labkey.api.data.Selector.ForEachBlock;
@@ -30,6 +31,7 @@
 import org.labkey.api.util.ExceptionUtil;
 import org.labkey.api.util.PageFlowUtil;
 import org.labkey.api.util.TestContext;
+import org.labkey.api.util.logging.LogHelper;
 import org.springframework.jdbc.UncategorizedSQLException;
 
 import java.sql.ResultSet;
@@ -50,6 +52,8 @@
 
 public class TableSelectorTestCase extends AbstractSelectorTestCase<TableSelector>
 {
+ private static final Logger LOG = LogHelper.getLogger(TableSelectorTestCase.class, "Test progress");
+
  @Test
  public void testTableSelector() throws SQLException
  {
@@ -59,13 +63,25 @@ public void testTableSelector() throws SQLException
 // testTableSelector(DbSchema.get("oracle.granite", DbSchemaType.Bare).getTable("account"), Account.class);
 
  // Test MySQL or MariaDB database, if present
- List<DbScope> mySqlScopes = Stream.of("mySql", "mariadb")
- .map(DbScope::getDbScope).filter(Objects::nonNull).toList();
+ List<DbScope> mySqlScopes = DbScope.getDbScopesToTest().stream()
+ .filter(scope -> Set.of("MySQL", "MariaDB").contains(scope.getSqlDialect().getProductName()))
+ .toList();
+
  for (DbScope mySqlScope: mySqlScopes)
  {
  DbSchema sakila = mySqlScope.getSchema("sakila", DbSchemaType.Bare);
  if (sakila.existsInDatabase())
- testTableSelector(sakila.getTable("country"), Country.class);
+ {
+ testTableSelector(sakila.getTable("Country"), Country.class);
+ }
+ else
+ {
+ DbSchema sys = mySqlScope.getSchema("sys", DbSchemaType.Bare);
+ if (sys.existsInDatabase())
+ {
+ testTableSelector(sys.getTable("sys_config"), Config.class);
+ }
+ }
  }
  testTableSelector(CoreSchema.getInstance().getTableInfoActiveUsers(), User.class);
  testTableSelector(CoreSchema.getInstance().getTableInfoModules(), ModuleContext.class);
@@ -124,6 +140,8 @@ public int hashCode()
  }
  }
 
+ record Config(String Variable, String Value, Date Set_Time, String Set_By){}
+
 // public static class Account
 // {
 // private int _account_id;
@@ -430,6 +448,9 @@ private void testColumnList(TableSelector selector, boolean stable) throws SQLEx
 
  private <K> void testTableSelector(TableInfo table, Class<K> clazz) throws SQLException
  {
+ DbSchema schema = table.getSchema();
+ LOG.info("Testing {}.{}.{}", schema.getScope().getDisplayName(), schema.getName(), table.getName());
+
  TableSelector selector = new TableSelector(table);
 
  test(selector, clazz);

api/src/org/labkey/api/data/dialect/StandardJdbcHelper.java

@@ -44,7 +44,7 @@ protected String parseDatabase(String url) throws ServletException
 if (-1 == dbEnd)
 dbEnd = url.length();
 
-// Last '/' is the database delimiter, except for "jdbc:postgresql:database"
+// Last '/' is the database delimiter, except for "jdbc:postgresql:database" and old Oracle formats
 chardbDelimiter = url.contains("/") ? '/' : ':';
 intdbDelimiterIndex = url.lastIndexOf(dbDelimiter, dbEnd);
 

core/src/org/labkey/core/CoreModule.java

@@ -1502,6 +1502,7 @@ public TabDisplayMode getTabDisplayMode()
  public @NotNull Set<Class<?>> getUnitTests()
  {
  return Set.of(
+ AdminController.FileRootPermissionTestCase.class,
  ApiJsonWriter.TestCase.class,
  ClassLoaderTestCase.class,
  CopyFileRootPipelineJob.TestCase.class,

core/src/org/labkey/core/admin/AdminController.java

@@ -6425,6 +6425,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;
@@ -6562,22 +6563,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)
@@ -9556,6 +9597,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;