diff --git a/survey/src/org/labkey/survey/SurveyController.java b/survey/src/org/labkey/survey/SurveyController.java index 396154e643e..e4e81d93a27 100644 --- a/survey/src/org/labkey/survey/SurveyController.java +++ b/survey/src/org/labkey/survey/SurveyController.java @@ -82,6 +82,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.IntFunction; import java.util.stream.Collectors; public class SurveyController extends SpringActionController implements SurveyUrls @@ -177,7 +178,7 @@ else if (form.getRowId() != null) form.setResponsesPk(survey.getResponsesPk()); form.setSubmitted(survey.getSubmitted() != null); - SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getSurveyDesignId()); + SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), form.getSurveyDesignId()); if (surveyDesign != null) { _title = (form.isSubmitted() ? "Review: " : "Update: ") + surveyDesign.getLabel(); @@ -186,7 +187,7 @@ else if (form.getRowId() != null) } else if (form.getSurveyDesignId() != null) { - SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getSurveyDesignId()); + SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), form.getSurveyDesignId()); if (surveyDesign == null) { errors.reject(ERROR_MSG, "Error: No SurveyDesign record found for rowId " + form.getSurveyDesignId() + "."); @@ -221,7 +222,7 @@ public ModelAndView getView(SurveyDesignForm form,BindException errors) { if (form.getRowId() != 0) { - SurveyDesign survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getRowId()); + SurveyDesign survey = SurveyManager.get().getSurveyDesignForWrite(getContainer(), getUser(), form.getRowId()); if (survey != null) _title = "Update Survey Design : " + survey.getLabel(); } @@ -340,10 +341,12 @@ public class SaveSurveyTemplateAction extends MutatingApiAction SurveyManager.get().getSurveyDesignForWrite(getContainer(), getUser(), id)); Map errorInfo = new HashMap<>(); - try { + try + { // try to validate the metadata String metadata = StringUtils.trimToNull(form.getMetadata()); @@ -416,13 +419,13 @@ else if (NumberUtils.isDigits(part)) // Should be positive integer } } - private SurveyDesign getSurveyDesign(SurveyDesignForm form) + private SurveyDesign getSurveyDesign(SurveyDesignForm form, IntFunction designResolver) { SurveyDesign survey = new SurveyDesign(); if (form.getRowId() != 0) { - survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getRowId()); - // getSurveyDesign is container-scoped; null here means the rowId doesn't belong to this folder + survey = designResolver.apply(form.getRowId()); + // null here means the rowId isn't accessible for this operation (e.g. a write from the wrong folder) if (survey == null) throw new NotFoundException("No survey design found for rowId " + form.getRowId() + " in this folder"); } @@ -430,7 +433,7 @@ else if (form.getDesignId() != null) { if (NumberUtils.isDigits(form.getDesignId())) { - survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), NumberUtils.toInt(form.getDesignId())); + survey = designResolver.apply(NumberUtils.toInt(form.getDesignId())); if (survey == null) throw new NotFoundException("No survey design found for designId " + form.getDesignId() + " in this folder"); } @@ -492,7 +495,8 @@ public class GetSurveyTemplateAction extends ReadOnlyApiAction public ApiResponse execute(SurveyDesignForm form, BindException errors) { ApiSimpleResponse response = new ApiSimpleResponse(); - SurveyDesign survey = getSurveyDesign(form); + // Reading tolerates a cross-container reference as long as the caller can read the design's own container. + SurveyDesign survey = getSurveyDesign(form, id -> SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), id)); if (survey != null) { @@ -519,7 +523,7 @@ public ApiResponse execute(SurveyResponseForm form, BindException errors) throws SurveyDesign surveyDesign = null; if (form.getSurveyDesignId() != null) - surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getSurveyDesignId()); + surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), form.getSurveyDesignId()); if (surveyDesign != null) { @@ -733,7 +737,7 @@ public ApiResponse execute(SurveyForm form, BindException errors) if (survey != null && !survey.isNew()) { - SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), survey.getSurveyDesignId()); + SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), survey.getSurveyDesignId()); if (surveyDesign != null) { @@ -857,7 +861,7 @@ public ApiResponse execute(SurveyAttachmentForm form, BindException errors) thro List files = getAttachmentFileList(); if (!files.isEmpty()) { - SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), survey.getSurveyDesignId()); + SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), survey.getSurveyDesignId()); if (surveyDesign != null) { diff --git a/survey/src/org/labkey/survey/SurveyManager.java b/survey/src/org/labkey/survey/SurveyManager.java index e5593ac3eec..1a3795c2a36 100644 --- a/survey/src/org/labkey/survey/SurveyManager.java +++ b/survey/src/org/labkey/survey/SurveyManager.java @@ -64,6 +64,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.AuthorRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.survey.model.Survey; import org.labkey.api.survey.model.SurveyDesign; @@ -71,6 +72,7 @@ import org.labkey.api.util.JsonUtil; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; +import org.labkey.api.view.ActionURL; import org.labkey.api.view.ViewContext; import org.springframework.validation.BindException; @@ -268,16 +270,36 @@ public Survey saveSurvey(Container container, User user, Survey survey) } } - /** Checks that the user has read permission to the container that owns the design, but we accept cross-container references */ + /** + * Checks that the user has read permission to the container that owns the design, but we accept cross-container references + */ + @Nullable + public SurveyDesign getSurveyDesignForRead(Container container, User user, int surveyId) + { + SurveyDesign surveyDesign = _getSurveyDesign(new SimpleFilter(), surveyId); + + if (surveyDesign != null) + { + Container actualContainer = ContainerManager.getForId(surveyDesign.getContainerId()); + + // A survey design can be requested from a different folder provided the user has read permission. + return actualContainer == null || !actualContainer.hasPermission(user, ReadPermission.class) ? null : surveyDesign; + } + return null; + } + + @Nullable + public SurveyDesign getSurveyDesignForWrite(Container container, User user, int surveyId) + { + // Container scoping is enforced when updating a survey design. + return _getSurveyDesign(SimpleFilter.createContainerFilter(container), surveyId); + } + @Nullable - public SurveyDesign getSurveyDesign(Container container, User user, int surveyId) + private SurveyDesign _getSurveyDesign(SimpleFilter filter, int surveyId) { - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("rowId"), surveyId); - SurveyDesign result = new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); - if (result == null) - return null; - Container actualContainer = ContainerManager.getForId(result.getContainerId()); - return actualContainer == null || !actualContainer.hasPermission(user, ReadPermission.class) ? null : result; + filter.addCondition(FieldKey.fromParts("rowId"), surveyId); + return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class); } /** @@ -462,7 +484,7 @@ public List fireBeforeDeleteSurvey(Container c, User user, Survey sur public static List fireDeleteSurvey(Container c, User user, Survey survey) { List errors = new ArrayList<>(); - SurveyDesign design = SurveyManager.get().getSurveyDesign(c, user, survey.getSurveyDesignId()); + SurveyDesign design = SurveyManager.get().getSurveyDesignForRead(c, user, survey.getSurveyDesignId()); for (SurveyListener l : _surveyListeners) { @@ -825,26 +847,59 @@ public void testSurveyDesignContainerScoping() throws Exception // 1. Same container: a user with read access in the design's container sees it. User readerA = createUserInRole(_projectA, ReaderRole.class); assertNotNull("Design should be visible from its own container to a user with read access", - sm.getSurveyDesign(_projectA, readerA, designId)); + sm.getSurveyDesignForRead(_projectA, readerA, designId)); // 2. Different container, caller can read the design's container: tolerated, design is returned. User readerAB = createUserInRole(_projectA, ReaderRole.class); grantRole(readerAB, _projectB, ReaderRole.class); assertNotNull("Design should be visible from another container when the caller can read the design's container", - sm.getSurveyDesign(_projectB, readerAB, designId)); + sm.getSurveyDesignForRead(_projectB, readerAB, designId)); // 3. Different container, caller cannot read the design's container: must return null. User readerB = createUserInRole(_projectB, ReaderRole.class); assertNull("Design must NOT be visible to a caller without read access to the design's container", - sm.getSurveyDesign(_projectB, readerB, designId)); + sm.getSurveyDesignForRead(_projectB, readerB, designId)); // A delete issued from the wrong container must not remove the design sm.deleteSurveyDesign(_projectB, _user, designId, true); - assertNotNull("Cross-container delete must be a no-op", sm.getSurveyDesign(_projectA, _user, designId)); + assertNotNull("Cross-container delete must be a no-op", sm.getSurveyDesignForRead(_projectA, _user, designId)); // A delete from the correct container removes it sm.deleteSurveyDesign(_projectA, _user, designId, true); - assertNull("Same-container delete should remove the design", sm.getSurveyDesign(_projectA, _user, designId)); + assertNull("Same-container delete should remove the design", sm.getSurveyDesignForRead(_projectA, _user, designId)); + } + + // GH Issue 1308: SaveSurveyTemplateAction must not let a caller in one folder overwrite and reparent a survey + // design owned by another folder. + @Test + public void testSaveSurveyTemplateActionContainerScoping() throws Exception + { + SurveyManager sm = SurveyManager.get(); + + SurveyDesign design = new SurveyDesign(); + design.setLabel("Design owned by A"); + design.setDescription("original description"); + design = sm.saveSurveyDesign(_projectA, _user, design); + int designId = design.getRowId(); + + User attacker = createUserInRole(_projectA, ReaderRole.class); + grantRole(attacker, _projectB, AuthorRole.class); + + ActionURL url = new ActionURL(SurveyController.SaveSurveyTemplateAction.class, _projectB) + .addParameter("rowId", designId) + .addParameter("label", "STOLEN") + .addParameter("description", "hijacked"); + post(url, attacker); + + // The design must still belong to folder A with its original field values: not reparented, not overwritten. + SurveyDesign after = sm.getSurveyDesignForRead(_projectA, _user, designId); + assertNotNull("Design must still exist after the cross-container save attempt", after); + assertEquals("Design must NOT be reparented into the attacker's container", + _projectA.getId(), after.getContainerId()); + assertEquals("Design label must NOT be overwritten from another container", + "Design owned by A", after.getLabel()); + assertEquals("Design description must NOT be overwritten from another container", + "original description", after.getDescription()); } @Test @@ -854,7 +909,7 @@ public void testSurveyContainerScoping() SurveyDesign design = new SurveyDesign(); design.setLabel("Scoping test design for survey"); - design = sm.saveSurveyDesign(_projectA, _user, design); + design = sm.saveSurveyDesign(_projectA, _user, design); Survey survey = new Survey(); survey.setLabel("Scoping test survey"); diff --git a/survey/src/org/labkey/survey/SurveyModule.java b/survey/src/org/labkey/survey/SurveyModule.java index 4976d189ff2..d72fe84786c 100644 --- a/survey/src/org/labkey/survey/SurveyModule.java +++ b/survey/src/org/labkey/survey/SurveyModule.java @@ -227,7 +227,7 @@ public WebPartView getWebPartView(@NotNull ViewContext context, @NotNull Port return new HtmlView("Surveys", HtmlString.of("There is no survey design selected to be displayed in this webpart.")); else { - surveyDesign = SurveyManager.get().getSurveyDesign(context.getContainer(), context.getUser(), Integer.parseInt(designIdStr)); + surveyDesign = SurveyManager.get().getSurveyDesignForRead(context.getContainer(), context.getUser(), Integer.parseInt(designIdStr)); if (surveyDesign == null) return new HtmlView("Surveys", HtmlString.of("The survey design configured for this webpart cannot be found and may have been deleted.")); diff --git a/survey/src/org/labkey/survey/model/SurveyServiceImpl.java b/survey/src/org/labkey/survey/model/SurveyServiceImpl.java index 5f3fa5c6265..1b3829099fe 100644 --- a/survey/src/org/labkey/survey/model/SurveyServiceImpl.java +++ b/survey/src/org/labkey/survey/model/SurveyServiceImpl.java @@ -64,7 +64,7 @@ public SurveyDesign[] getSurveyDesigns(SimpleFilter filter) @Override public SurveyDesign getSurveyDesign(Container container, User user, int surveyDesignId) { - return SurveyManager.get().getSurveyDesign(container, user, surveyDesignId); + return SurveyManager.get().getSurveyDesignForRead(container, user, surveyDesignId); } @Override