From ffb38ec387760e00d127494fb9bb734bced239f7 Mon Sep 17 00:00:00 2001 From: ankurjuneja Date: Wed, 9 Sep 2026 18:52:47 -0700 Subject: [PATCH 1/2] Check DeletePermission per job in DeleteStatusAction.getView --- .../pipeline/status/StatusController.java | 59 ++++++++++++++++++- .../labkey/pipeline/status/deleteStatus.jsp | 10 ++-- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index da705b2bb65..00926a9a50e 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -108,6 +108,7 @@ import static org.labkey.pipeline.api.PipelineStatusManager.completeStatus; import static org.labkey.pipeline.api.PipelineStatusManager.deleteStatus; import static org.labkey.pipeline.api.PipelineStatusManager.getStatusFile; +import static org.labkey.pipeline.api.PipelineStatusManager.getStatusFiles; public class StatusController extends SpringActionController @@ -745,6 +746,17 @@ public static class ConfirmDeleteStatusForm extends SelectStatusForm private String _dataRegionSelectionKey; private boolean _confirm; private boolean _deleteRuns; + private List _statusFiles = List.of(); + + public List getStatusFiles() + { + return _statusFiles; + } + + public void setStatusFiles(List statusFiles) + { + _statusFiles = statusFiles; + } public String getDataRegionSelectionKey() { @@ -777,7 +789,7 @@ public void setDeleteRuns(boolean deleteRuns) } } - // DeletePermission will be checked in PipelineStatusManager.deleteStatus() + // DeletePermission is checked per job, against the job's own container, in getView() and in PipelineStatusManager.deleteStatus() @RequiresPermission(ReadPermission.class) public class DeleteStatusAction extends FormViewAction { @@ -801,6 +813,18 @@ public void validateCommand(ConfirmDeleteStatusForm form, Errors errors) @Override public ModelAndView getView(ConfirmDeleteStatusForm form, boolean reshow, BindException errors) { + getContainerCheckAdmin(); + + int[] rowIds = form.getRowIds() == null ? new int[0] : form.getRowIds(); + List statusFiles = getStatusFiles(rowIds); + for (PipelineStatusFileImpl sf : statusFiles) + { + Container sfContainer = sf.lookupContainer(); + if (sfContainer == null || !sfContainer.hasPermission(getUser(), DeletePermission.class)) + throw new NotFoundException("Could not find status file for rowId " + sf.getRowId()); + } + form.setStatusFiles(statusFiles); + return new JspView<>("/org/labkey/pipeline/status/deleteStatus.jsp", form, errors); } @@ -1239,6 +1263,39 @@ public void testDetailsContainerScoping() throws Exception assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); } + @Test + public void testDeleteStatusContainerScoping() throws Exception + { + User admin = getAdmin(); + Container folderA = createContainer("A"); + Container folderB = createContainer("B"); + User readerA = createUserInRole(folderA, ReaderRole.class); + User readerB = createUserInRole(folderB, ReaderRole.class); + + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()).getRowId(); + String rowIdParam = String.valueOf(rowId); + + // A caller with no rights in folder B must not see B's job described through folder A + ActionURL foreignUrl = new ActionURL(DeleteStatusAction.class, folderA).addParameter("rowIds", rowIdParam); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, readerA)); + + // Same on the POST that reshows the page, where the rowIds come from the data region selection + ActionURL selectUrl = new ActionURL(DeleteStatusAction.class, folderA).addParameter(DataRegion.SELECT_CHECKBOX_NAME, rowIdParam); + assertStatus(HttpServletResponse.SC_NOT_FOUND, post(selectUrl, readerA)); + + // Read alone isn't enough: the page confirms a delete, so it requires DeletePermission like the POST does + ActionURL ownUrl = new ActionURL(DeleteStatusAction.class, folderB).addParameter("rowIds", rowIdParam); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(ownUrl, readerB)); + + // Positive controls: a caller who can delete in B gets the page through either container -- a selection can + // legitimately span containers when the grid uses a container filter. + assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); + assertStatus(HttpServletResponse.SC_OK, get(foreignUrl, admin)); + + // Rendering the confirmation page must not delete anything + assertNotNull("Job must still exist after rendering the confirmation page", getStatusFile(rowId)); + } + // Insert a bare status file in the given container. FilePath is a required column; point it at a non-existent // log so nothing tries to read it, and so getJobStore().getJob() returns null (exercising the bean-only paths). private PipelineStatusFileImpl insertStatusFile(Container c, String status) diff --git a/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp b/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp index fbbfb7a6973..ee2b6a50a76 100644 --- a/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp +++ b/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp @@ -23,6 +23,7 @@ <%@ page import="org.labkey.api.pipeline.PipeRoot" %> <%@ page import="org.labkey.api.pipeline.PipelineService" %> <%@ page import="org.labkey.api.pipeline.PipelineStatusUrls" %> +<%@ page import="org.labkey.api.security.permissions.DeletePermission" %> <%@ page import="org.labkey.api.util.FileUtil" %> <%@ page import="org.labkey.api.util.NetworkDrive" %> <%@ page import="org.labkey.api.view.ActionURL" %> @@ -114,6 +115,9 @@ sb.append("
    "); for (PipelineStatusFileImpl child : children) { + Container childContainer = child.lookupContainer(); + if (childContainer == null || !childContainer.hasPermission(getUser(), DeletePermission.class)) + continue; sb.append(renderStatusFile(root, child, allRuns)); } sb.append("
"); @@ -131,10 +135,8 @@ PipeRoot root = PipelineService.get().findPipelineRoot(getContainer()); - int[] rowIds = form.getRowIds(); - if (rowIds == null) - rowIds = new int[0]; - List files = PipelineStatusManager.getStatusFiles(rowIds); + // Already permission-checked by DeleteStatusAction.getView(); don't re-query by the client-supplied rowIds + List files = form.getStatusFiles(); Set allRuns = new LinkedHashSet<>(); %> From 8a5e13bdcd044b30b9e3f1d6a5ccdcaf15c80b7b Mon Sep 17 00:00:00 2001 From: ankurjuneja Date: Thu, 10 Sep 2026 17:20:44 -0700 Subject: [PATCH 2/2] Scope delete confirmation to jobs the delete will reach, and delete what it displays Report failures instead of silently skipping rows the container-filtered DELETE can't reach. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01C44xjpwocskmf8ndHXRpcw --- .../pipeline/api/PipelineStatusManager.java | 30 ++++++++++-- .../pipeline/status/StatusController.java | 48 ++++++++++++++++--- .../labkey/pipeline/status/deleteStatus.jsp | 9 ++-- 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java index 8655f30669b..8e5dc443373 100644 --- a/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java +++ b/pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java @@ -124,6 +124,27 @@ public static List getStatusFiles(int... rowId) return new TableSelector(_schema.getTableInfoStatusFiles(), filter, null).getArrayList(PipelineStatusFileImpl.class); } + /** + * Get the PipelineStatusFileImpls with the given RowIds that are reachable from c, using + * the same container scope that {@link #deleteStatus} deletes within. Rows outside that scope are not returned. + */ + public static List getStatusFilesInScope(Container c, User user, int... rowIds) + { + if (rowIds.length == 0) + return Collections.emptyList(); + + SQLFragment sql = new SQLFragment("SELECT * FROM ").append(_schema.getTableInfoStatusFiles()).append(" WHERE RowId "); + _schema.getSqlDialect().appendInClauseSql(sql, Arrays.stream(rowIds).boxed().collect(Collectors.toList())); + + if (!c.isRoot()) + { + sql.append(" AND "); + sql.append(ContainerFilter.current(c, user).getSQLFragment(_schema.getSchema(), new SQLFragment("Container"))); + } + + return new SqlSelector(_schema.getSchema(), sql).getArrayList(PipelineStatusFileImpl.class); + } + /** * Get a PipelineStatusFileImpl by the file path associated with the * entry. @@ -808,9 +829,6 @@ private static void deleteStatus(Container container, User user, boolean deleteE _schema.getSqlDialect().appendInClauseSql(sql, statusFileIds); _schema.getSqlDialect().appendInClauseSql(expSql, statusFileIds); - // Remember that we deleted these rows - statusFileIds.forEach(rowIds::remove); - if (!container.isRoot()) { // Use a ContainerFilter to generate the SQL so that we include workbooks - see issue 22236 @@ -829,6 +847,12 @@ private static void deleteStatus(Container container, User user, boolean deleteE int rowCount = new SqlExecutor(_schema.getSchema()).execute(sql); + // Only forget the rows the DELETE actually reached: the container filter above can exclude a job in + // another container, and it has to stay in rowIds so the caller reports a failure instead of success + SimpleFilter survivorFilter = new SimpleFilter(new SimpleFilter.InClause(FieldKey.fromParts("RowId"), statusFileIds)); + Set survivors = new HashSet<>(new TableSelector(_schema.getTableInfoStatusFiles(), Collections.singleton("RowId"), survivorFilter, null).getArrayList(Long.class)); + statusFileIds.stream().filter(id -> !survivors.contains(id)).forEach(rowIds::remove); + // If we deleted anything, try recursing since we may have deleted all the child jobs which would // allow a parent job to be deleted if (rowCount > 0 && !rowIds.isEmpty()) diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index 00926a9a50e..4904ae5f8c7 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -99,6 +99,7 @@ import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; +import java.util.Arrays; import java.util.Date; import java.util.List; import java.util.Set; @@ -108,7 +109,7 @@ import static org.labkey.pipeline.api.PipelineStatusManager.completeStatus; import static org.labkey.pipeline.api.PipelineStatusManager.deleteStatus; import static org.labkey.pipeline.api.PipelineStatusManager.getStatusFile; -import static org.labkey.pipeline.api.PipelineStatusManager.getStatusFiles; +import static org.labkey.pipeline.api.PipelineStatusManager.getStatusFilesInScope; public class StatusController extends SpringActionController @@ -796,6 +797,10 @@ public class DeleteStatusAction extends FormViewAction @Override public void validateCommand(ConfirmDeleteStatusForm form, Errors errors) { + // The confirmation page posts back the rowIds it displayed; only fall back to the grid selection + if (form.getRowIds() != null && form.getRowIds().length > 0) + return; + // Don't clear the state yet because we're just validating at this point. We'll clear it as part of the // delete itself. See issue 44873 Set runs = DataRegionSelection.getSelected(getViewContext(), false); @@ -813,10 +818,14 @@ public void validateCommand(ConfirmDeleteStatusForm form, Errors errors) @Override public ModelAndView getView(ConfirmDeleteStatusForm form, boolean reshow, BindException errors) { - getContainerCheckAdmin(); + Container c = getContainerCheckAdmin(); int[] rowIds = form.getRowIds() == null ? new int[0] : form.getRowIds(); - List statusFiles = getStatusFiles(rowIds); + // Scope to what deleteStatus() would delete, so the page can't list jobs the confirm wouldn't touch + List statusFiles = getStatusFilesInScope(c, getUser(), rowIds); + if (statusFiles.size() != Arrays.stream(rowIds).distinct().count()) + throw new NotFoundException("Could not find status file for every requested rowId"); + for (PipelineStatusFileImpl sf : statusFiles) { Container sfContainer = sf.lookupContainer(); @@ -835,15 +844,23 @@ public boolean handlePost(ConfirmDeleteStatusForm form, BindException errors) return false; getContainerCheckAdmin(); + + // Delete the jobs the confirmation page listed, not a selection that may since have changed + Set rowIds = new TreeSet<>(); + for (int rowId : form.getRowIds() == null ? new int[0] : form.getRowIds()) + rowIds.add((long) rowId); + try { - deleteStatus(getViewBackgroundInfo().getContainer(), getViewBackgroundInfo().getUser(), form.isDeleteRuns(), DataRegionSelection.getSelectedIntegers(getViewContext(), true)); + deleteStatus(getViewBackgroundInfo().getContainer(), getViewBackgroundInfo().getUser(), form.isDeleteRuns(), rowIds); } catch (PipelineProvider.HandlerException e) { errors.addError(new LabKeyError(e.getMessage() == null ? "Failed to delete at least one job. It may be referenced by other jobs" : e.getMessage())); return false; } + + DataRegionSelection.clearAll(getViewContext()); return true; } @@ -1287,15 +1304,32 @@ public void testDeleteStatusContainerScoping() throws Exception ActionURL ownUrl = new ActionURL(DeleteStatusAction.class, folderB).addParameter("rowIds", rowIdParam); assertStatus(HttpServletResponse.SC_NOT_FOUND, get(ownUrl, readerB)); - // Positive controls: a caller who can delete in B gets the page through either container -- a selection can - // legitimately span containers when the grid uses a container filter. + // Even a site admin gets 404 through folder A: deleteStatus() scopes its DELETE to the URL's container, so + // listing B's job here would promise a delete that silently wouldn't happen. + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreignUrl, admin)); + + // Positive control: through its own container a caller who can delete gets the page assertStatus(HttpServletResponse.SC_OK, get(ownUrl, admin)); - assertStatus(HttpServletResponse.SC_OK, get(foreignUrl, admin)); // Rendering the confirmation page must not delete anything assertNotNull("Job must still exist after rendering the confirmation page", getStatusFile(rowId)); } + @Test + public void testDeleteStatusActsOnDisplayedJobs() throws Exception + { + // The page lists form.getRowIds(), so the confirm must delete those same ids rather than a grid selection + // that the confirming request may not carry at all. + Container folderB = createContainer("B"); + long rowId = insertStatusFile(folderB, PipelineJob.TaskStatus.complete.toString()).getRowId(); + + ActionURL confirmUrl = new ActionURL(DeleteStatusAction.class, folderB) + .addParameter("rowIds", String.valueOf(rowId)) + .addParameter("confirm", "true"); + assertStatus(HttpServletResponse.SC_FOUND, post(confirmUrl, getAdmin())); + assertNull("Confirming must delete the job the page listed", getStatusFile(rowId)); + } + // Insert a bare status file in the given container. FilePath is a required column; point it at a non-existent // log so nothing tries to read it, and so getJobStore().getJob() returns null (exercising the bean-only paths). private PipelineStatusFileImpl insertStatusFile(Container c, String status) diff --git a/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp b/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp index ee2b6a50a76..b978976394a 100644 --- a/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp +++ b/pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp @@ -16,7 +16,6 @@ */ %> <%@ page import="org.labkey.api.data.Container" %> -<%@ page import="org.labkey.api.data.DataRegion" %> <%@ page import="org.labkey.api.data.DataRegionSelection" %> <%@ page import="org.labkey.api.exp.api.ExpRun" %> <%@ page import="org.labkey.api.exp.api.ExperimentService" %> @@ -176,12 +175,10 @@

<% - if (getViewContext().getRequest().getParameterValues(DataRegion.SELECT_CHECKBOX_NAME) != null) + // Post back exactly the jobs listed above, so the delete acts on what was confirmed + for (PipelineStatusFileImpl file : files) { - for (String selectedValue : getViewContext().getRequest().getParameterValues(DataRegion.SELECT_CHECKBOX_NAME)) - { - %><% - } + %><% } %>