From 7574aefb441a8a9441f46c42f1a98b540b3fc699 Mon Sep 17 00:00:00 2001 From: ankurjuneja Date: Sun, 13 Sep 2026 14:12:19 -0700 Subject: [PATCH 1/3] Guard material writes in ExpGeneratorHelper with UpdatePermission --- .../experiment/pipeline/ExpGeneratorHelper.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java b/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java index 36b6e74d216..2506f6676a4 100644 --- a/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java +++ b/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java @@ -50,7 +50,9 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.util.FileUtil; +import org.labkey.api.view.UnauthorizedException; import org.labkey.experiment.api.ExpDataImpl; import org.labkey.experiment.api.ExpMaterialImpl; import org.labkey.experiment.api.ExpRunImpl; @@ -278,6 +280,14 @@ static public ExpRunImpl insertRun(Container container, User user, return run; } + // Require write access to a material's own container before mutating its lineage. + private static void assertCanModifyMaterial(User user, ExpMaterial material) + { + if (material != null && !material.getContainer().hasPermission(user, UpdatePermission.class)) + throw new UnauthorizedException("No permission to modify sample '" + material.getName() + + "' in " + material.getContainer().getPath()); + } + static private ExpRunImpl _insertRun(Container container, User user, String runName, @@ -377,6 +387,7 @@ else if (action.isEnd()) for (String lsid : action.getMaterialInputs()) { ExpMaterial material = ExperimentService.get().getExpMaterial(lsid); + assertCanModifyMaterial(user, material); material.setRun(run); stepApp.addMaterialInput(user, material, null, null); } @@ -385,6 +396,7 @@ else if (action.isEnd()) for (String lsid : action.getMaterialOutputs()) { ExpMaterialImpl material = (ExpMaterialImpl) ExperimentService.get().getExpMaterial(lsid); + assertCanModifyMaterial(user, material); material.setSourceApplication(stepApp); // set up the output to the run if (action.isEnd()) From 35de23371e6598c64a14a95ee0586aa233c42b65 Mon Sep 17 00:00:00 2001 From: ankurjuneja Date: Mon, 14 Sep 2026 16:59:43 -0700 Subject: [PATCH 2/3] claude code review changes --- .../pipeline/ExpGeneratorHelper.java | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java b/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java index 2506f6676a4..74f73467190 100644 --- a/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java +++ b/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java @@ -15,6 +15,7 @@ */ package org.labkey.experiment.pipeline; +import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -38,6 +39,7 @@ import org.labkey.api.exp.api.ExpRun; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.api.ProvenanceService; +import org.labkey.api.exp.api.SampleTypeService; import org.labkey.api.pipeline.PipelineJob; import org.labkey.api.pipeline.PipelineJobException; import org.labkey.api.pipeline.PipelineJobService; @@ -50,8 +52,9 @@ import org.labkey.api.query.FieldKey; import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; -import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.util.FileUtil; +import org.labkey.api.view.NotFoundException; import org.labkey.api.view.UnauthorizedException; import org.labkey.experiment.api.ExpDataImpl; import org.labkey.experiment.api.ExpMaterialImpl; @@ -74,6 +77,8 @@ */ public class ExpGeneratorHelper { + private static final Logger LOG = LogManager.getLogger(ExpGeneratorHelper.class); + static private ExpData addData(Container container, User user, Map datas, URI originalURI, XarSource source) throws ExperimentException { ExpData data = datas.get(originalURI); @@ -280,12 +285,20 @@ static public ExpRunImpl insertRun(Container container, User user, return run; } - // Require write access to a material's own container before mutating its lineage. - private static void assertCanModifyMaterial(User user, ExpMaterial material) + // Unresolved and unauthorized both return NotFoundException, so a foreign LSID is never confirmed. + private static void assertCanEditLineage(User user, String lsid, ExpMaterial material) { - if (material != null && !material.getContainer().hasPermission(user, UpdatePermission.class)) - throw new UnauthorizedException("No permission to modify sample '" + material.getName() - + "' in " + material.getContainer().getPath()); + Class permission = SampleTypeService.SampleOperations.EditLineage.getPermissionClass(); + if (material == null || (permission != null && !material.getContainer().hasPermission(user, permission))) + { + if (material != null) + LOG.warn("User {} cannot edit lineage of material {} in {}", user, lsid, material.getContainer().getPath()); + throw new NotFoundException("Could not find material with LSID '" + lsid + "'"); + } + + if (!material.isOperationPermitted(SampleTypeService.SampleOperations.EditLineage)) + throw new UnauthorizedException(SampleTypeService.get().getOperationNotPermittedMessage( + List.of(material), SampleTypeService.SampleOperations.EditLineage)); } static private ExpRunImpl _insertRun(Container container, @@ -383,20 +396,19 @@ else if (action.isEnd()) stepApp.setProperty(user, pd, prop.getValue()); } - // material inputs + // material inputs - only adds an edge, never rewrites the material, so no write check for (String lsid : action.getMaterialInputs()) { ExpMaterial material = ExperimentService.get().getExpMaterial(lsid); - assertCanModifyMaterial(user, material); material.setRun(run); stepApp.addMaterialInput(user, material, null, null); } - // material outputs + // material outputs - these rewrite the material's lineage, so require edit rights for (String lsid : action.getMaterialOutputs()) { ExpMaterialImpl material = (ExpMaterialImpl) ExperimentService.get().getExpMaterial(lsid); - assertCanModifyMaterial(user, material); + assertCanEditLineage(user, lsid, material); material.setSourceApplication(stepApp); // set up the output to the run if (action.isEnd()) @@ -549,6 +561,7 @@ static private void promoteInputs(Set actions, ExpRun run, Map Date: Wed, 16 Sep 2026 16:58:46 -0700 Subject: [PATCH 3/3] code review comments --- .../experiment/pipeline/ExpGeneratorHelper.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java b/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java index 74f73467190..0557667ae1b 100644 --- a/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java +++ b/experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java @@ -53,6 +53,7 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.security.permissions.Permission; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.util.FileUtil; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.UnauthorizedException; @@ -285,6 +286,14 @@ static public ExpRunImpl insertRun(Container container, User user, return run; } + private static ExpMaterial resolveReadableMaterial(User user, String lsid) + { + ExpMaterial material = ExperimentService.get().getExpMaterial(lsid); + if (material == null || !material.getContainer().hasPermission(user, ReadPermission.class)) + throw new NotFoundException("Could not find material with LSID '" + lsid + "'"); + return material; + } + // Unresolved and unauthorized both return NotFoundException, so a foreign LSID is never confirmed. private static void assertCanEditLineage(User user, String lsid, ExpMaterial material) { @@ -396,10 +405,10 @@ else if (action.isEnd()) stepApp.setProperty(user, pd, prop.getValue()); } - // material inputs - only adds an edge, never rewrites the material, so no write check + // material inputs - adds an edge for (String lsid : action.getMaterialInputs()) { - ExpMaterial material = ExperimentService.get().getExpMaterial(lsid); + ExpMaterial material = resolveReadableMaterial(user, lsid); material.setRun(run); stepApp.addMaterialInput(user, material, null, null); } @@ -561,10 +570,10 @@ static private void promoteInputs(Set actions, ExpRun run, Map