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;