diff --git a/src/org/labkey/targetedms/SkylineAuditLogManager.java b/src/org/labkey/targetedms/SkylineAuditLogManager.java index a3d9077f7..15aa690da 100644 --- a/src/org/labkey/targetedms/SkylineAuditLogManager.java +++ b/src/org/labkey/targetedms/SkylineAuditLogManager.java @@ -606,7 +606,7 @@ public void testEntryRetrieval() throws AuditLogException { AuditLogTree node = new SkylineAuditLogManager(_container, null).buildLogTree(_docGUID); while(node.iterator().hasNext()){ - AuditLogEntry ent = AuditLogEntry.retrieve(node.getEntryId()); + AuditLogEntry ent = AuditLogEntry.retrieve(node.getEntryId(), _container); assertNotNull(ent); node = node.iterator().next(); } diff --git a/src/org/labkey/targetedms/TargetedMSContainerScopingTest.java b/src/org/labkey/targetedms/TargetedMSContainerScopingTest.java new file mode 100644 index 000000000..cb6d13999 --- /dev/null +++ b/src/org/labkey/targetedms/TargetedMSContainerScopingTest.java @@ -0,0 +1,143 @@ +/* + * Copyright (c) 2026 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.targetedms; + +import jakarta.servlet.http.HttpServletResponse; +import org.apache.logging.log4j.Logger; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.Table; +import org.labkey.api.data.TableSelector; +import org.labkey.api.exp.api.ExpRun; +import org.labkey.api.exp.api.ExperimentService; +import org.labkey.api.pipeline.PipeRoot; +import org.labkey.api.pipeline.PipelineService; +import org.labkey.api.query.FieldKey; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.util.GUID; +import org.labkey.api.util.logging.LogHelper; +import org.labkey.api.view.ActionURL; +import org.labkey.api.view.ViewBackgroundInfo; +import org.labkey.targetedms.parser.skyaudit.UnitTestUtil; + +import java.io.File; +import java.util.Collections; +import java.util.List; +import java.util.Set; + +/** + * Container-scoping integration tests for {@link TargetedMSController} actions that resolve an object by a + * global id. Each test sets up the same object in one folder and confirms the action rejects a request that addresses + * via the wrong container + */ +public class TargetedMSContainerScopingTest extends AbstractContainerScopingTest +{ + private static final Logger LOG = LogHelper.getLogger(TargetedMSContainerScopingTest.class, "TargetedMS container scoping tests"); + private static final GUID DOC_GUID = new GUID("8f1c2a44-3c5e-4f0a-9d2b-6e7a1b3c5d9e"); + + @Before + @After + public void cleanupAuditLog() + { + UnitTestUtil.cleanupDatabase(DOC_GUID); + } + + /** + * RemoveLinkVersionAction relinks a Skyline document-version chain by run rowId. It must reject a run that lives in + * a different container (mirroring its sibling SaveLinkVersionsAction), otherwise an editor in one folder could + * rewrite the version chain of documents they can't see. + */ + @Test + public void removeLinkVersionIsContainerScoped() throws Exception + { + Container owner = createContainer("LinkOwner"); + Container other = createContainer("LinkOther"); + + // Negative: a run that lives in 'owner' cannot be relinked through the 'other' folder + ExpRun foreignRun = createRun(owner, "foreign-version"); + ActionURL foreign = new ActionURL(TargetedMSController.RemoveLinkVersionAction.class, other) + .addParameter("rowId", foreignRun.getRowId()); + assertStatus(HttpServletResponse.SC_BAD_REQUEST, post(foreign, getAdmin())); + + // Positive: a run that lives in the acting folder is accepted + ExpRun localRun = createRun(other, "local-version"); + ActionURL local = new ActionURL(TargetedMSController.RemoveLinkVersionAction.class, other) + .addParameter("rowId", localRun.getRowId()); + assertStatus(HttpServletResponse.SC_OK, post(local, getAdmin())); + } + + /** + * ShowSkylineAuditLogExtraInfoAJAXAction renders the full Skyline audit detail (user, timestamp, extra info) for an + * audit entry resolved by global EntryId. It must only render entries whose owning run is in the requested folder, + * otherwise any reader could read another folder's document audit history by guessing entry ids. + */ + @Test + public void auditLogExtraInfoIsContainerScoped() throws Exception + { + Container owner = createContainer("AuditOwner"); + Container other = createContainer("AuditOther"); + + int entryId = importAuditLogEntry(owner); + + // Negative: the entry's run lives in 'owner', so requesting it via 'other' must 404 rather than leak detail + ActionURL foreign = new ActionURL(TargetedMSController.ShowSkylineAuditLogExtraInfoAJAXAction.class, other) + .addParameter("entryId", entryId); + assertStatus(HttpServletResponse.SC_NOT_FOUND, get(foreign, getAdmin())); + + // Positive: requesting the entry through its owning folder renders normally + ActionURL owned = new ActionURL(TargetedMSController.ShowSkylineAuditLogExtraInfoAJAXAction.class, owner) + .addParameter("entryId", entryId); + assertStatus(HttpServletResponse.SC_OK, get(owned, getAdmin())); + } + + /** Persist a minimal, saved {@link ExpRun} in the given container so the controller can resolve it by rowId. */ + private ExpRun createRun(Container c, String name) throws Exception + { + ExperimentService svc = ExperimentService.get(); + ExpRun run = svc.createExperimentRun(c, name); + PipeRoot pipeRoot = PipelineService.get().findPipelineRoot(c); + assertNotNull("No pipeline root for " + c.getPath(), pipeRoot); + run.setFilePathRoot(pipeRoot.getRootPath()); + run.setProtocol(svc.ensureSampleDerivationProtocol(getAdmin())); + ViewBackgroundInfo info = new ViewBackgroundInfo(c, getAdmin(), null); + return svc.saveSimpleExperimentRun(run, Collections.emptyMap(), Collections.emptyMap(), + Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap(), info, null, false); + } + + /** Insert a TargetedMS run in the given container, import a sample Skyline audit log for it, and return an EntryId. */ + private int importAuditLogEntry(Container c) throws Exception + { + TargetedMSRun run = new TargetedMSRun(); + run.setContainer(c); + run.setDocumentGUID(DOC_GUID); + Table.insert(getAdmin(), TargetedMSManager.getTableInfoRuns(), run); + + File zip = UnitTestUtil.getSampleDataFile("AuditLogFiles/MethodEdit_v1.zip"); + File logFile = UnitTestUtil.extractLogFromZip(zip, LOG); + new SkylineAuditLogManager(c, null).importAuditLogFile(getAdmin(), logFile, DOC_GUID, run); + + List entryIds = new TableSelector( + TargetedMSManager.getTableInfoSkylineRunAuditLogEntry(), + Set.of("AuditLogEntryId"), + new SimpleFilter(FieldKey.fromParts("VersionId"), run.getId()), null) + .getArrayList(Integer.class); + assertFalse("No audit log entries were imported", entryIds.isEmpty()); + return entryIds.get(0); + } +} diff --git a/src/org/labkey/targetedms/TargetedMSController.java b/src/org/labkey/targetedms/TargetedMSController.java index 58719e055..85c04b12d 100644 --- a/src/org/labkey/targetedms/TargetedMSController.java +++ b/src/org/labkey/targetedms/TargetedMSController.java @@ -4957,7 +4957,9 @@ public static class ShowSkylineAuditLogExtraInfoAJAXAction extends SimpleViewAct @Override public ModelAndView getView(SkylineAuditLogExtraInfoForm form, BindException errors) { - AuditLogEntry ent = AuditLogEntry.retrieve(form.getEntryId()); + AuditLogEntry ent = AuditLogEntry.retrieve(form.getEntryId(), getContainer()); + if (ent == null) + throw new NotFoundException("No audit log entry found for id " + form.getEntryId() + " in this folder."); getPageConfig().setTemplate(PageConfig.Template.None); return new JspView<>("/org/labkey/targetedms/view/skylineAuditLogExtraInfoView.jsp", ent); } @@ -7657,7 +7659,7 @@ public void validateForm(RowIdForm form, Errors errors) // verify that the run rowId is valid and matches an existing run // and if the run replaces any other runs, it should only replace one ExpRun run = ExperimentService.get().getExpRun(form.getRowId()); - if (run == null) + if (run == null || !run.getContainer().equals(getContainer())) errors.reject(ERROR_MSG, "No run found for id " + form.getRowId() + "."); else if (!run.getReplacesRuns().isEmpty() && run.getReplacesRuns().size() > 1) errors.reject(ERROR_MSG, "Run " + form.getRowId() + " replaces more than one run."); diff --git a/src/org/labkey/targetedms/TargetedMSModule.java b/src/org/labkey/targetedms/TargetedMSModule.java index d0469a0a9..533785b62 100644 --- a/src/org/labkey/targetedms/TargetedMSModule.java +++ b/src/org/labkey/targetedms/TargetedMSModule.java @@ -687,7 +687,8 @@ public Set getIntegrationTests() { return Set.of( MsDataSourceUtil.TestCase.class, - SkylineAuditLogManager.TestCase.class + SkylineAuditLogManager.TestCase.class, + TargetedMSContainerScopingTest.class ); } diff --git a/src/org/labkey/targetedms/outliers/OutlierGenerator.java b/src/org/labkey/targetedms/outliers/OutlierGenerator.java index 6d5ff96ec..994da0615 100644 --- a/src/org/labkey/targetedms/outliers/OutlierGenerator.java +++ b/src/org/labkey/targetedms/outliers/OutlierGenerator.java @@ -26,6 +26,7 @@ import org.labkey.api.data.TableSelector; import org.labkey.api.query.QueryService; import org.labkey.api.security.User; +import org.labkey.api.sql.LabKeySql; import org.labkey.api.targetedms.model.SampleFileInfo; import org.labkey.api.util.Pair; import org.labkey.targetedms.TargetedMSManager; @@ -112,7 +113,7 @@ private String getEachSeriesTypePlotDataSql(int seriesIndex, QCMetricConfigurati sql.append(" CAST(IFDEFINED(SeriesLabel) AS VARCHAR) AS SeriesLabel, "); sql.append("\nMetricValue, 0 as metric, ").append(seriesIndex).append(" AS MetricSeriesIndex, ").append(configuration.getId()).append(" AS MetricId"); - sql.append("\n FROM ").append(schemaName).append('.').append(queryName); + sql.append("\n FROM ").append(schemaName).append('.').append(LabKeySql.quoteIdentifier(queryName)); if (!annotationGroups.isEmpty()) { diff --git a/src/org/labkey/targetedms/parser/skyaudit/AuditLogEntry.java b/src/org/labkey/targetedms/parser/skyaudit/AuditLogEntry.java index 2d0e35445..3743c1e6d 100644 --- a/src/org/labkey/targetedms/parser/skyaudit/AuditLogEntry.java +++ b/src/org/labkey/targetedms/parser/skyaudit/AuditLogEntry.java @@ -15,6 +15,7 @@ */ package org.labkey.targetedms.parser.skyaudit; +import org.labkey.api.data.Container; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Table; import org.labkey.api.data.TableSelector; @@ -22,6 +23,7 @@ import org.labkey.api.security.User; import org.labkey.api.util.GUID; import org.labkey.targetedms.TargetedMSManager; +import org.labkey.targetedms.TargetedMSRun; import java.math.BigDecimal; import java.math.BigInteger; @@ -70,13 +72,25 @@ public AuditLogEntry(BigDecimal documentFormatVersion) _documentFormatVersion = documentFormatVersion; } - public static AuditLogEntry retrieve(int pEntryId) + /** + * Container-scoped retrieve. Returns the entry only if its owning Skyline run lives in the supplied container, + * preventing callers from reading audit detail for documents in folders the user can't access. An entryId can + * map to more than one run when documents share an audit history, so check every match for one in the container. + */ + public static AuditLogEntry retrieve(int pEntryId, Container container) { TableSelector sel = new TableSelector(TargetedMSManager.getTableInfoSkylineAuditLog(), new SimpleFilter(FieldKey.fromParts("EntryId"), pEntryId), null); - List results = sel.getArrayList(AuditLogEntry.class); - // Possible to get more than one match if two documents share an audit history. In this case, we don't care - // which we use - return results.isEmpty() ? null : results.get(0); + for (AuditLogEntry entry : sel.getArrayList(AuditLogEntry.class)) + { + Long runId = entry.getRunId(); + if (runId != null) + { + TargetedMSRun run = TargetedMSManager.getRun(runId); + if (run != null && container.equals(run.getContainer())) + return entry; + } + } + return null; } public AuditLogTree getTreeEntry(){ diff --git a/test/src/org/labkey/test/tests/targetedms/passport/PassportTest.java b/test/src/org/labkey/test/tests/targetedms/passport/PassportTest.java index 76554c4d2..c85b5c3b2 100644 --- a/test/src/org/labkey/test/tests/targetedms/passport/PassportTest.java +++ b/test/src/org/labkey/test/tests/targetedms/passport/PassportTest.java @@ -24,6 +24,8 @@ import java.util.List; +import static org.junit.Assert.assertTrue; + @Category({}) public class PassportTest extends PassportTestPart { @@ -91,13 +93,20 @@ private void testNormalStuff() waitForElement(Locator.tagWithId("span", "filteredPeptideCount").childTag("green")); assertElementContains(Locator.xpath("//span[@id='filteredPeptideCount']//green"), "19"); - //features - waitForElements(Locator.tagWithClass("td", "feature-sequencevariant"), 5); - waitForElements(Locator.tagWithClass("td", "feature-glycosylationsite"), 4); - waitForElements(Locator.tagWithClass("td", "feature-helix"), 7); - waitForElements(Locator.tagWithClass("td", "feature-turn"), 6); + // Features come from a live query against Uniprot, so be tolerant of any number above our baseline + waitForElementsAtLeast(Locator.tagWithClass("td", "feature-sequencevariant"), 3); + waitForElementsAtLeast(Locator.tagWithClass("td", "feature-glycosylationsite"), 3); + waitForElementsAtLeast(Locator.tagWithClass("td", "feature-helix"), 3); + waitForElementsAtLeast(Locator.tagWithClass("td", "feature-turn"), 3); } + public void waitForElementsAtLeast(final Locator loc, final int count) + { + waitFor(() -> loc.findElements(getDriver()).size() >= count, WAIT_FOR_JAVASCRIPT); + assertTrue("Element not present at least the expected number of times", loc.findElements(getDriver()).size() >= count); + } + + @LogMethod protected void testAsNormalUser() {