From d7888e35767c355197ee6aca9b1f509a1bd7b0ae Mon Sep 17 00:00:00 2001 From: Lum Date: Tue, 27 Oct 2026 10:51:02 -0700 Subject: [PATCH 1/5] Validation of script report permissions via study controller actions. --- .../api/reports/report/view/ReportUtil.java | 3 +- .../query/reports/ReportServiceImpl.java | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/api/reports/report/view/ReportUtil.java b/api/src/org/labkey/api/reports/report/view/ReportUtil.java index 17f0acec074..5ac44d6ec41 100644 --- a/api/src/org/labkey/api/reports/report/view/ReportUtil.java +++ b/api/src/org/labkey/api/reports/report/view/ReportUtil.java @@ -79,6 +79,7 @@ import org.labkey.api.view.ActionURL; import org.labkey.api.view.TabStripView; import org.labkey.api.view.ViewContext; +import org.labkey.api.writer.ContainerUser; import org.springframework.validation.Errors; import javax.script.ScriptEngine; @@ -351,7 +352,7 @@ public static boolean isReportInherited(Container c, Report report) /** * Helper to determine if the user can create a script report. */ - public static boolean canCreateScript(ViewContext context, String extension) + public static boolean canCreateScript(ContainerUser context, String extension) { User u = context.getUser(); diff --git a/query/src/org/labkey/query/reports/ReportServiceImpl.java b/query/src/org/labkey/query/reports/ReportServiceImpl.java index 93e006be423..9875b9615ce 100644 --- a/query/src/org/labkey/query/reports/ReportServiceImpl.java +++ b/query/src/org/labkey/query/reports/ReportServiceImpl.java @@ -46,6 +46,7 @@ import org.labkey.api.data.TableSelector; import org.labkey.api.module.Module; import org.labkey.api.moduleeditor.api.ModuleEditorService; +import org.labkey.api.premium.PremiumFeatureNotEnabledException; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.query.SimpleValidationError; @@ -59,6 +60,7 @@ import org.labkey.api.reports.model.ViewCategoryManager; import org.labkey.api.reports.report.AbstractReportIdentifier; import org.labkey.api.reports.report.DbReportIdentifier; +import org.labkey.api.reports.report.JavaScriptReport; import org.labkey.api.reports.report.ModuleJavaScriptReportDescriptor; import org.labkey.api.reports.report.ModuleReportDescriptor; import org.labkey.api.reports.report.ModuleReportIdentifier; @@ -67,7 +69,9 @@ import org.labkey.api.reports.report.ReportIdentifier; import org.labkey.api.reports.report.ReportIdentifierConverter; import org.labkey.api.reports.report.ScriptEngineReport; +import org.labkey.api.reports.report.ScriptReport; import org.labkey.api.reports.report.ScriptReportDescriptor; +import org.labkey.api.reports.report.python.IpynbReport; import org.labkey.api.reports.report.python.ModuleIpynbReportDescriptor; import org.labkey.api.reports.report.r.ModuleRReportDescriptor; import org.labkey.api.reports.report.r.RReportDescriptor; @@ -102,6 +106,7 @@ import org.labkey.query.xml.ReportDescriptorDocument; import org.labkey.query.xml.ReportDescriptorType; +import javax.script.ScriptEngine; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -389,6 +394,7 @@ public boolean tryValidateReportPermissions(ContainerUser context, Report report if (descriptor.isNew()) { + validateScriptCreation(context, report, errors); if (descriptor.isShared()) report.canShare(context.getUser(), context.getContainer(), errors); } @@ -413,6 +419,39 @@ public boolean tryValidateReportPermissions(ContainerUser context, Report report return errors.isEmpty(); } + private void validateScriptCreation(ContainerUser context, Report report, List errors) + { + if (!(report instanceof ScriptReport)) + return; + + String extension = null; + if (report instanceof ScriptEngineReport scriptEngineReport) + { + try + { + ScriptEngine engine = scriptEngineReport.getScriptEngine(context.getContainer()); + if (engine == null) + { + errors.add(new SimpleValidationError("Unable to find a configured script engine for this report.")); + return; + } + extension = engine.getFactory().getExtensions().getFirst(); + } + catch (PremiumFeatureNotEnabledException e) + { + errors.add(new SimpleValidationError(e.getMessage())); + return; + } + } + else if (report instanceof JavaScriptReport) + extension = "js"; + else if (report instanceof IpynbReport) + extension = IpynbReport.EXTENSION; + + if (!ReportUtil.canCreateScript(context, extension)) + errors.add(new SimpleValidationError("You must be either a PlatformDeveloper or TrustedAnalyst to create a script report.")); + } + private ReportDB _saveDbReport(ContainerUser context, String key, Report report, boolean skipValidation) { DbScope scope = getTable().getSchema().getScope(); From 7b55cb43b18631033a03ba0bada846b5f3165736 Mon Sep 17 00:00:00 2001 From: lum Date: Fri, 30 Oct 2026 13:51:21 -0700 Subject: [PATCH 2/5] simplify implementation --- .../api/reports/report/view/ReportUtil.java | 13 ++++++- .../query/reports/ReportServiceImpl.java | 39 ++++++++----------- 2 files changed, 28 insertions(+), 24 deletions(-) diff --git a/api/src/org/labkey/api/reports/report/view/ReportUtil.java b/api/src/org/labkey/api/reports/report/view/ReportUtil.java index 5ac44d6ec41..f253bb3536c 100644 --- a/api/src/org/labkey/api/reports/report/view/ReportUtil.java +++ b/api/src/org/labkey/api/reports/report/view/ReportUtil.java @@ -353,6 +353,17 @@ public static boolean isReportInherited(Container c, Report report) * Helper to determine if the user can create a script report. */ public static boolean canCreateScript(ContainerUser context, String extension) + { + LabKeyScriptEngineManager svc = LabKeyScriptEngineManager.get(); + ScriptEngine engine = svc.getEngineByExtension(context.getContainer(), extension); + + return canCreateScript(context, engine); + } + + /** + * Helper to determine if the user can create a script report. + */ + public static boolean canCreateScript(ContainerUser context, ScriptEngine engine) { User u = context.getUser(); @@ -367,8 +378,6 @@ public static boolean canCreateScript(ContainerUser context, String extension) { // trusted analysts can only create reports on sandboxed engine instances // TODO can this differ from the engine returned by the instantiated report? - LabKeyScriptEngineManager svc = LabKeyScriptEngineManager.get(); - ScriptEngine engine = svc.getEngineByExtension(context.getContainer(), extension); return engine instanceof LabKeyScriptEngine && ((LabKeyScriptEngine)engine).isSandboxed(); } diff --git a/query/src/org/labkey/query/reports/ReportServiceImpl.java b/query/src/org/labkey/query/reports/ReportServiceImpl.java index f1b7c4698a0..fff7eb345c7 100644 --- a/query/src/org/labkey/query/reports/ReportServiceImpl.java +++ b/query/src/org/labkey/query/reports/ReportServiceImpl.java @@ -47,12 +47,12 @@ import org.labkey.api.data.TableSelector; import org.labkey.api.module.Module; import org.labkey.api.moduleeditor.api.ModuleEditorService; -import org.labkey.api.premium.PremiumFeatureNotEnabledException; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.query.SimpleValidationError; import org.labkey.api.query.ValidationError; import org.labkey.api.query.ValidationException; +import org.labkey.api.reports.LabKeyScriptEngineManager; import org.labkey.api.reports.Report; import org.labkey.api.reports.ReportService; import org.labkey.api.reports.model.ReportPropsManager; @@ -426,31 +426,26 @@ private void validateScriptCreation(ContainerUser context, Report report, List Date: Mon, 2 Nov 2026 15:23:34 -0800 Subject: [PATCH 3/5] Restrict report types and raise permission for the study SaveReportAction. --- .../query/reports/ReportServiceImpl.java | 6 ++- .../reports/ReportsController.java | 52 ++++++++++++++----- .../VisualizationController.java | 2 +- 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/query/src/org/labkey/query/reports/ReportServiceImpl.java b/query/src/org/labkey/query/reports/ReportServiceImpl.java index fff1e203b5b..73e2e3726ee 100644 --- a/query/src/org/labkey/query/reports/ReportServiceImpl.java +++ b/query/src/org/labkey/query/reports/ReportServiceImpl.java @@ -412,6 +412,10 @@ public boolean tryValidateReportPermissions(ContainerUser context, Report report return false; } + // canEdit checks the stored report, so a save that changes the type must pass the new type's creation check + if (null == stored || !stored.getType().equals(report.getType())) + validateScriptCreation(context, report, errors); + Report toCheck = null != stored ? stored : report; if (toCheck.canEdit(context.getUser(), context.getContainer(), errors)) @@ -421,7 +425,7 @@ public boolean tryValidateReportPermissions(ContainerUser context, Report report } } - // GH Issue 1649: generic report actions, such as study-reports-saveReport, can write a chart's config too + // GH Issue 1649: generic report actions, such as study-reports-saveReportView, can write a chart's config too descriptor.validateContent(context.getUser(), null != stored ? stored.getDescriptor() : null, errors); return errors.isEmpty(); diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index adbc5d3db7d..fe8c550e473 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -65,7 +65,9 @@ import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.Role; import org.labkey.api.study.Dataset; import org.labkey.api.study.Study; import org.labkey.api.study.StudyService; @@ -102,6 +104,7 @@ import org.labkey.study.query.StudyQuerySchema; import org.labkey.study.reports.ParticipantReport; import org.labkey.study.reports.ReportManager; +import org.labkey.study.reports.StudyQueryReport; import org.labkey.vfs.FileLike; import org.springframework.validation.BindException; import org.springframework.validation.Errors; @@ -180,18 +183,17 @@ public void addNavTrail(NavTree root) } } - @RequiresLogin - @RequiresPermission(ReadPermission.class) - /* - Action for non-query based views (static, xls export) - */ + // Saves dataset grid views from createQueryReport.jsp + @RequiresPermission(AdminPermission.class) public class SaveReportAction extends FormHandlerAction { private ActionURL _successURL; @Override - public void validateCommand(SaveReportForm target, Errors errors) + public void validateCommand(SaveReportForm form, Errors errors) { + if (!StudyQueryReport.TYPE.equals(form.getReportType())) + errors.reject(ERROR_MSG,"Unsupported report type: " + form.getReportType()); } @Override @@ -1350,10 +1352,34 @@ public void testShowReportRequiresReadPermission() throws Exception @Test public void testOverwritingSavedReports() throws Exception { - assertReportNotStolen(SaveReportAction.class, false); + assertReportNotStolen(SaveReportAction.class, FolderAdminRole.class, false); // shareReport leaves the descriptor's owner null, so the save is checked as a shared report and the // caller's Editor role in their own folder satisfies the only permission the save path consults. - assertReportNotStolen(SaveReportViewAction.class, true); + assertReportNotStolen(SaveReportViewAction.class, EditorRole.class, true); + } + + @Test + public void testUpdateCannotConvertToScriptReport() throws Exception + { + Container folder = createContainer("A"); + + Report gridReport = ReportService.get().createReportInstance(QueryReport.TYPE); + gridReport.getDescriptor().setReportName("grid-to-script-report"); + int rowId = ReportService.get() + .saveReportEx(new DefaultContainerUser(folder, getAdmin()), "grid-to-script-key", gridReport) + .getRowId(); + + User editor = createUserInRole(folder, EditorRole.class); + ActionURL url = new ActionURL(SaveReportViewAction.class, folder) + .addParameter("reportType", "Study.rReport") + .addParameter("label", "grid-to-script-report") + .addParameter("shareReport", "true") + .addParameter("params", "reportId=db%3A" + rowId + "&script=print%281%29"); + post(url, editor); + + Report stored = ReportService.get().getReport(folder, rowId); + assertNotNull(stored); + assertEquals("An Editor converted a grid report into a script report", QueryReport.TYPE, stored.getType()); } /** @@ -1361,7 +1387,7 @@ public void testOverwritingSavedReports() throws Exception * verbatim into its property map -- reportId included -- so the save targets a row of the caller's choosing. * The shared path then authorizes against the request container and updates by primary key alone. */ - private void assertReportNotStolen(Class action, boolean shareReport) throws Exception + private void assertReportNotStolen(Class action, Class attackerRole, boolean shareReport) throws Exception { Container attackerFolder = createContainer("A"); Container victimFolder = createContainer("B"); @@ -1373,10 +1399,10 @@ private void assertReportNotStolen(Class action, boolean s .saveReportEx(new DefaultContainerUser(victimFolder, getAdmin()), "scoping-victim-key", victimReport) .getRowId(); - User attacker = createUserInRole(attackerFolder, EditorRole.class); + User attacker = createUserInRole(attackerFolder, attackerRole); ActionURL url = new ActionURL(action, attackerFolder) - .addParameter("reportType", QueryReport.TYPE) + .addParameter("reportType", StudyQueryReport.TYPE) .addParameter("label", "scoping-stolen-report") .addParameter("params", "reportId=db%3A" + victimRowId); if (shareReport) @@ -1412,7 +1438,6 @@ public void testActionPermissions() assertForReadPermission(user, false, controller.new BeginAction(), new StreamFileAction(), - controller.new SaveReportAction(), controller.new SaveReportViewAction(), new ShowReportAction(), new ParticipantCrosstabAction(), @@ -1424,7 +1449,8 @@ controller.new ParticipantReportAction(), // @RequiresPermission(AdminPermission.class) assertForAdminPermission(user, - controller.new CreateQueryReportAction() + controller.new CreateQueryReportAction(), + controller.new SaveReportAction() ); } diff --git a/visualization/src/org/labkey/visualization/VisualizationController.java b/visualization/src/org/labkey/visualization/VisualizationController.java index fd3ca010aad..6b96ea1fd2f 100644 --- a/visualization/src/org/labkey/visualization/VisualizationController.java +++ b/visualization/src/org/labkey/visualization/VisualizationController.java @@ -1838,7 +1838,7 @@ private MockHttpServletResponse saveStudyReport(Container folder, User user, Str Pair.of(ReportDescriptor.Prop.schemaName.name(), "core"), Pair.of(ReportDescriptor.Prop.queryName.name(), "Users"), Pair.of(GenericChartReportDescriptor.Prop.renderType.name(), "scatter_plot"))); - ActionURL url = new ActionURL("study-reports", "saveReport", folder) + ActionURL url = new ActionURL("study-reports", "saveReportView", folder) .addParameter("reportType", GenericChartReport.TYPE) .addParameter("label", label) .addParameter("params", params); From d262e551a8ca8baec52fe315346122f42dfbefa2 Mon Sep 17 00:00:00 2001 From: Lum Date: Mon, 2 Nov 2026 15:48:26 -0800 Subject: [PATCH 4/5] don't allow report type change --- query/src/org/labkey/query/reports/ReportServiceImpl.java | 2 +- .../org/labkey/study/controllers/reports/ReportsController.java | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/query/src/org/labkey/query/reports/ReportServiceImpl.java b/query/src/org/labkey/query/reports/ReportServiceImpl.java index 73e2e3726ee..66f11980ffd 100644 --- a/query/src/org/labkey/query/reports/ReportServiceImpl.java +++ b/query/src/org/labkey/query/reports/ReportServiceImpl.java @@ -413,7 +413,7 @@ public boolean tryValidateReportPermissions(ContainerUser context, Report report } // canEdit checks the stored report, so a save that changes the type must pass the new type's creation check - if (null == stored || !stored.getType().equals(report.getType())) + if (descriptor.getReportId() instanceof DbReportIdentifier && (null == stored || !stored.getType().equals(report.getType()))) validateScriptCreation(context, report, errors); Report toCheck = null != stored ? stored : report; diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index fe8c550e473..9a407c5ac25 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -766,6 +766,8 @@ public Report getReport(ContainerUser cu) ReportDescriptor descriptor = report.getDescriptor(); descriptor.setReportName(label); descriptor.initFromQueryString(params); + // params can carry a reportType, which would load the saved row as a different report class + descriptor.setReportType(reportType); descriptor.setProperty("showWithDataset", String.valueOf(showWithDataset)); descriptor.setReportDescription(description); descriptor.setProperty(ReportDescriptor.Prop.dataRegionName, dataRegionName); From 671757e065b837358bc82fc8972fbb7c38c848ad Mon Sep 17 00:00:00 2001 From: Lum Date: Mon, 2 Nov 2026 16:22:25 -0800 Subject: [PATCH 5/5] restrict the study saveReportView to crosstab reports --- .../reports/ReportsController.java | 14 +++++++---- .../VisualizationController.java | 24 ------------------- 2 files changed, 9 insertions(+), 29 deletions(-) diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index 9a407c5ac25..f4edebb2dd8 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -104,6 +104,7 @@ import org.labkey.study.query.StudyQuerySchema; import org.labkey.study.reports.ParticipantReport; import org.labkey.study.reports.ReportManager; +import org.labkey.study.reports.StudyCrosstabReport; import org.labkey.study.reports.StudyQueryReport; import org.labkey.vfs.FileLike; import org.springframework.validation.BindException; @@ -279,7 +280,10 @@ public ModelAndView getView(SaveReportViewForm form, boolean reshow, BindExcepti @Override public void validateCommand(SaveReportViewForm form, Errors errors) { - if (ReportService.get().reportNameExists(getViewContext(), form.getLabel(), ReportUtil.getReportKey(form.getSchemaName(), form.getQueryName()))) + // Saves crosstab reports from the participantCrosstab designer + if (!StudyCrosstabReport.TYPE.equals(form.getReportType())) + errors.reject(ERROR_MSG, "Unsupported report type: " + form.getReportType()); + else if (ReportService.get().reportNameExists(getViewContext(), form.getLabel(), ReportUtil.getReportKey(form.getSchemaName(), form.getQueryName()))) errors.reject("saveReportView", "There is already a report with the name of: '" + form.getLabel() + "'. Please specify a different name."); } @@ -1354,10 +1358,10 @@ public void testShowReportRequiresReadPermission() throws Exception @Test public void testOverwritingSavedReports() throws Exception { - assertReportNotStolen(SaveReportAction.class, FolderAdminRole.class, false); + assertReportNotStolen(SaveReportAction.class, StudyQueryReport.TYPE, FolderAdminRole.class, false); // shareReport leaves the descriptor's owner null, so the save is checked as a shared report and the // caller's Editor role in their own folder satisfies the only permission the save path consults. - assertReportNotStolen(SaveReportViewAction.class, EditorRole.class, true); + assertReportNotStolen(SaveReportViewAction.class, StudyCrosstabReport.TYPE, EditorRole.class, true); } @Test @@ -1389,7 +1393,7 @@ public void testUpdateCannotConvertToScriptReport() throws Exception * verbatim into its property map -- reportId included -- so the save targets a row of the caller's choosing. * The shared path then authorizes against the request container and updates by primary key alone. */ - private void assertReportNotStolen(Class action, Class attackerRole, boolean shareReport) throws Exception + private void assertReportNotStolen(Class action, String reportType, Class attackerRole, boolean shareReport) throws Exception { Container attackerFolder = createContainer("A"); Container victimFolder = createContainer("B"); @@ -1404,7 +1408,7 @@ private void assertReportNotStolen(Class action, Class