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..f253bb3536c 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,18 @@ 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) + { + 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(); @@ -366,8 +378,6 @@ public static boolean canCreateScript(ViewContext 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 35e506066c7..66f11980ffd 100644 --- a/query/src/org/labkey/query/reports/ReportServiceImpl.java +++ b/query/src/org/labkey/query/reports/ReportServiceImpl.java @@ -49,6 +49,7 @@ 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; @@ -57,6 +58,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.ModuleReportDescriptor; import org.labkey.api.reports.report.ModuleReportIdentifier; import org.labkey.api.reports.report.ReportDB; @@ -64,7 +66,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.r.ModuleRReportDescriptor; import org.labkey.api.reports.report.r.RReportDescriptor; import org.labkey.api.reports.report.view.ReportUtil; @@ -99,6 +103,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; @@ -394,6 +399,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); } @@ -406,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 (descriptor.getReportId() instanceof DbReportIdentifier && (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)) @@ -415,12 +425,40 @@ 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(); } + private void validateScriptCreation(ContainerUser context, Report report, List errors) + { + if (!(report instanceof ScriptReport)) + return; + + ScriptEngine engine = null; + if (report instanceof ScriptEngineReport scriptEngineReport) + { + engine = scriptEngineReport.getScriptEngine(context.getContainer()); + } + else if (report instanceof JavaScriptReport || report instanceof IpynbReport) + { + String extension = (report instanceof JavaScriptReport) ? "js" : IpynbReport.EXTENSION; + + LabKeyScriptEngineManager svc = LabKeyScriptEngineManager.get(); + engine = svc.getEngineByExtension(context.getContainer(), extension); + } + + if (engine == null) + { + errors.add(new SimpleValidationError("Unable to find a configured script engine for this report.")); + return; + } + + if (!ReportUtil.canCreateScript(context, engine)) + 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(); diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index adbc5d3db7d..f4edebb2dd8 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,8 @@ 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; import org.springframework.validation.Errors; @@ -180,18 +184,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 @@ -277,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."); } @@ -764,6 +770,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); @@ -1350,10 +1358,34 @@ public void testShowReportRequiresReadPermission() throws Exception @Test public void testOverwritingSavedReports() throws Exception { - assertReportNotStolen(SaveReportAction.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, true); + assertReportNotStolen(SaveReportViewAction.class, StudyCrosstabReport.TYPE, 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 +1393,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, String reportType, Class attackerRole, boolean shareReport) throws Exception { Container attackerFolder = createContainer("A"); Container victimFolder = createContainer("B"); @@ -1373,10 +1405,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", reportType) .addParameter("label", "scoping-stolen-report") .addParameter("params", "reportId=db%3A" + victimRowId); if (shareReport) @@ -1412,7 +1444,6 @@ public void testActionPermissions() assertForReadPermission(user, false, controller.new BeginAction(), new StreamFileAction(), - controller.new SaveReportAction(), controller.new SaveReportViewAction(), new ShowReportAction(), new ParticipantCrosstabAction(), @@ -1424,7 +1455,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..d42d33fe7e2 100644 --- a/visualization/src/org/labkey/visualization/VisualizationController.java +++ b/visualization/src/org/labkey/visualization/VisualizationController.java @@ -1784,16 +1784,6 @@ public void testTimeChartRequiresTrustedDeveloper() throws Exception assertStatus(HttpServletResponse.SC_OK, saveTimeChart(folder, author, "time-developer-fn", new JSONObject().put("pointClickFn", FN))); } - @Test - public void testStudySaveReportRequiresTrustedDeveloper() throws Exception - { - Container folder = getSharedContainer("StudyCharts", ModuleLoader.getInstance().getModule(VisualizationModule.NAME), ModuleLoader.getInstance().getModule("study")); - User author = createUserInRole(folder, AuthorRole.class); - - assertStatus(HttpServletResponse.SC_FORBIDDEN, saveStudyReport(folder, author, "study-author-fn", genericChartJson(FN))); - assertStatus(HttpServletResponse.SC_FOUND, saveStudyReport(folder, author, "study-author-no-fn", genericChartJson(null))); - } - private static String genericChartJson(@Nullable String pointClickFn) { JSONObject measures = new JSONObject(); @@ -1830,19 +1820,5 @@ private MockHttpServletResponse saveTimeChart(Container folder, User user, Strin .put("json", config.toString()); return postJson(new ActionURL(SaveVisualizationAction.class, folder), user, body); } - - private MockHttpServletResponse saveStudyReport(Container folder, User user, String label, String json) throws Exception - { - String params = PageFlowUtil.toQueryString(List.of( - Pair.of(ReportDescriptor.Prop.json.name(), json), - 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) - .addParameter("reportType", GenericChartReport.TYPE) - .addParameter("label", label) - .addParameter("params", params); - return post(url, user); - } } }