Skip to content

Check DeletePermission per job in DeleteStatusAction.getView #8035

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

Open
ankurjuneja wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Check DeletePermission per job in DeleteStatusAction.getView #8035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffb38ec
Check DeletePermission per job in DeleteStatusAction.getView
ankurjuneja Sep 10, 2026
8a5e13b
Scope delete confirmation to jobs the delete will reach, and delete w…
ankurjuneja Sep 11, 2026
a0c44cb
Merge branch 'develop' into fb_pipelineDeleteStatus
ankurjuneja Sep 11, 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
30 changes: 27 additions & 3 deletions pipeline/src/org/labkey/pipeline/api/PipelineStatusManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,27 @@ public static List<PipelineStatusFileImpl> getStatusFiles(int... rowId)
return new TableSelector(_schema.getTableInfoStatusFiles(), filter, null).getArrayList(PipelineStatusFileImpl.class);
}

/**
* Get the <code>PipelineStatusFileImpl</code>s with the given RowIds that are reachable from <code>c</code>, using
* the same container scope that {@link #deleteStatus} deletes within. Rows outside that scope are not returned.
*/
public static List<PipelineStatusFileImpl> 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 <code>PipelineStatusFileImpl</code> by the file path associated with the
* entry.
Expand Down Expand Up @@ -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
Expand All @@ -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<Long> 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())
Expand Down
95 changes: 93 additions & 2 deletions pipeline/src/org/labkey/pipeline/status/StatusController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -108,6 +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.getStatusFilesInScope;


public class StatusController extends SpringActionController
Expand Down Expand Up @@ -745,6 +747,17 @@ public static class ConfirmDeleteStatusForm extends SelectStatusForm
private String _dataRegionSelectionKey;
private boolean _confirm;
private boolean _deleteRuns;
private List<PipelineStatusFileImpl> _statusFiles = List.of();

public List<PipelineStatusFileImpl> getStatusFiles()
{
return _statusFiles;
}

public void setStatusFiles(List<PipelineStatusFileImpl> statusFiles)
{
_statusFiles = statusFiles;
}

public String getDataRegionSelectionKey()
{
Expand Down Expand Up @@ -777,13 +790,17 @@ 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<ConfirmDeleteStatusForm>
{
@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<String> runs = DataRegionSelection.getSelected(getViewContext(), false);
Expand All @@ -801,6 +818,22 @@ public void validateCommand(ConfirmDeleteStatusForm form, Errors errors)
@Override
public ModelAndView getView(ConfirmDeleteStatusForm form, boolean reshow, BindException errors)
{
Container c = getContainerCheckAdmin();

int[] rowIds = form.getRowIds() == null ? new int[0] : form.getRowIds();
// Scope to what deleteStatus() would delete, so the page can't list jobs the confirm wouldn't touch
List<PipelineStatusFileImpl> 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();
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);
}

Expand All @@ -811,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<Long> 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;
}

Expand Down Expand Up @@ -1239,6 +1280,56 @@ 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));

// 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));

// 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)
Expand Down
19 changes: 9 additions & 10 deletions pipeline/src/org/labkey/pipeline/status/deleteStatus.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
*/
%>
<%@ 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" %>
<%@ 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" %>
Expand Down Expand Up @@ -114,6 +114,9 @@
sb.append("<ul>");
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("</ul>");
Expand All @@ -131,10 +134,8 @@

PipeRoot root = PipelineService.get().findPipelineRoot(getContainer());

int[] rowIds = form.getRowIds();
if (rowIds == null)
rowIds = new int[0];
List<PipelineStatusFileImpl> files = PipelineStatusManager.getStatusFiles(rowIds);
// Already permission-checked by DeleteStatusAction.getView(); don't re-query by the client-supplied rowIds
List<PipelineStatusFileImpl> files = form.getStatusFiles();

Set<ExpRun> allRuns = new LinkedHashSet<>();
%>
Expand Down Expand Up @@ -174,12 +175,10 @@
<p>
<input type="hidden" name="confirm" value="true">
<%
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))
{
%><input type="hidden" name="<%= h(DataRegion.SELECT_CHECKBOX_NAME) %>" value="<%= h(selectedValue) %>" /><%
}
%><input type="hidden" name="rowIds" value="<%= file.getRowId() %>" /><%
}
%>

Expand Down