Skip to content

Validation of script report permissions via study controller actions. #8252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
labkey-klum merged 7 commits into from
Nov 3, 2026
Merged

Validation of script report permissions via study controller actions. #8252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7888e3
Validation of script report permissions via study controller actions.
labkey-klum Oct 27, 2026
5d990ab
merge from develop
labkey-klum Oct 30, 2026
7b55cb4
simplify implementation
labkey-klum Oct 30, 2026
9651f70
Merge branch 'develop' into fb_study_script_report
labkey-klum Nov 2, 2026
6a6221d
Restrict report types and raise permission for the study SaveReportAc…
labkey-klum Nov 2, 2026
d262e55
don't allow report type change
labkey-klum Nov 2, 2026
671757e
restrict the study saveReportView to crosstab reports
labkey-klum Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions api/src/org/labkey/api/reports/report/view/ReportUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Expand All @@ -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();
}

Expand Down
40 changes: 39 additions & 1 deletion query/src/org/labkey/query/reports/ReportServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -57,14 +58,17 @@
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;
import org.labkey.api.reports.report.ReportDescriptor;
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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -394,6 +399,7 @@ public boolean tryValidateReportPermissions(ContainerUser context, Report report

if (descriptor.isNew())
{
validateScriptCreation(context, report, errors);

@labkey-matthewb labkey-matthewb Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why inside isNew() check?

@labkey-klum labkey-klum Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For existing reports, the permission check happens inside of canEdit

if (descriptor.isShared())
report.canShare(context.getUser(), context.getContainer(), errors);
}
Expand All @@ -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))
Expand All @@ -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<ValidationError> 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();
Expand Down
60 changes: 46 additions & 14 deletions study/src/org/labkey/study/controllers/reports/ReportsController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<SaveReportForm>
{
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
Expand Down Expand Up @@ -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.");
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1350,18 +1358,42 @@ 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());
}

/**
* Both save actions seed the descriptor from the client-supplied params string, which ReportDescriptor copies
* 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<? extends Controller> action, boolean shareReport) throws Exception
private void assertReportNotStolen(Class<? extends Controller> action, String reportType, Class<? extends Role> attackerRole, boolean shareReport) throws Exception
{
Container attackerFolder = createContainer("A");
Container victimFolder = createContainer("B");
Expand All @@ -1373,10 +1405,10 @@ private void assertReportNotStolen(Class<? extends Controller> 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)
Expand Down Expand Up @@ -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(),
Expand All @@ -1424,7 +1455,8 @@ controller.new ParticipantReportAction(),

// @RequiresPermission(AdminPermission.class)
assertForAdminPermission(user,
controller.new CreateQueryReportAction()
controller.new CreateQueryReportAction(),
controller.new SaveReportAction()
);
}

Expand Down
24 changes: 0 additions & 24 deletions visualization/src/org/labkey/visualization/VisualizationController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
}
Loading