Skip to content

Commit 297af23

Browse files
committed
Container scoping for survey save template action.
1 parent 5feb5cd commit 297af23

4 files changed

Lines changed: 89 additions & 30 deletions

File tree

survey/src/org/labkey/survey/SurveyController.java

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
import java.util.HashMap;
8383
import java.util.List;
8484
import java.util.Map;
85+
import java.util.function.IntFunction;
8586
import java.util.stream.Collectors;
8687

8788
public class SurveyController extends SpringActionController implements SurveyUrls
@@ -177,7 +178,7 @@ else if (form.getRowId() != null)
177178
form.setResponsesPk(survey.getResponsesPk());
178179
form.setSubmitted(survey.getSubmitted() != null);
179180

180-
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getSurveyDesignId());
181+
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), form.getSurveyDesignId());
181182
if (surveyDesign != null)
182183
{
183184
_title = (form.isSubmitted() ? "Review: " : "Update: ") + surveyDesign.getLabel();
@@ -186,7 +187,7 @@ else if (form.getRowId() != null)
186187
}
187188
else if (form.getSurveyDesignId() != null)
188189
{
189-
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getSurveyDesignId());
190+
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), form.getSurveyDesignId());
190191
if (surveyDesign == null)
191192
{
192193
errors.reject(ERROR_MSG, "Error: No SurveyDesign record found for rowId " + form.getSurveyDesignId() + ".");
@@ -221,7 +222,7 @@ public ModelAndView getView(SurveyDesignForm form,BindException errors)
221222
{
222223
if (form.getRowId() != 0)
223224
{
224-
SurveyDesign survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getRowId());
225+
SurveyDesign survey = SurveyManager.get().getSurveyDesignForWrite(getContainer(), getUser(), form.getRowId());
225226
if (survey != null)
226227
_title = "Update Survey Design : " + survey.getLabel();
227228
}
@@ -340,10 +341,12 @@ public class SaveSurveyTemplateAction extends MutatingApiAction<SurveyDesignForm
340341
public ApiResponse execute(SurveyDesignForm form, BindException errors) throws Exception
341342
{
342343
ApiSimpleResponse response = new ApiSimpleResponse();
343-
SurveyDesign survey = getSurveyDesign(form);
344+
// Updating the survey design. Resolve the design with container scoping.
345+
SurveyDesign survey = getSurveyDesign(form, id -> SurveyManager.get().getSurveyDesignForWrite(getContainer(), getUser(), id));
344346
Map<String, Object> errorInfo = new HashMap<>();
345347

346-
try {
348+
try
349+
{
347350
// try to validate the metadata
348351
String metadata = StringUtils.trimToNull(form.getMetadata());
349352

@@ -416,21 +419,21 @@ else if (NumberUtils.isDigits(part)) // Should be positive integer
416419
}
417420
}
418421

419-
private SurveyDesign getSurveyDesign(SurveyDesignForm form)
422+
private SurveyDesign getSurveyDesign(SurveyDesignForm form, IntFunction<SurveyDesign> designResolver)
420423
{
421424
SurveyDesign survey = new SurveyDesign();
422425
if (form.getRowId() != 0)
423426
{
424-
survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getRowId());
425-
// getSurveyDesign is container-scoped; null here means the rowId doesn't belong to this folder
427+
survey = designResolver.apply(form.getRowId());
428+
// null here means the rowId isn't accessible for this operation (e.g. a write from the wrong folder)
426429
if (survey == null)
427430
throw new NotFoundException("No survey design found for rowId " + form.getRowId() + " in this folder");
428431
}
429432
else if (form.getDesignId() != null)
430433
{
431434
if (NumberUtils.isDigits(form.getDesignId()))
432435
{
433-
survey = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), NumberUtils.toInt(form.getDesignId()));
436+
survey = designResolver.apply(NumberUtils.toInt(form.getDesignId()));
434437
if (survey == null)
435438
throw new NotFoundException("No survey design found for designId " + form.getDesignId() + " in this folder");
436439
}
@@ -492,7 +495,8 @@ public class GetSurveyTemplateAction extends ReadOnlyApiAction<SurveyDesignForm>
492495
public ApiResponse execute(SurveyDesignForm form, BindException errors)
493496
{
494497
ApiSimpleResponse response = new ApiSimpleResponse();
495-
SurveyDesign survey = getSurveyDesign(form);
498+
// Reading tolerates a cross-container reference as long as the caller can read the design's own container.
499+
SurveyDesign survey = getSurveyDesign(form, id -> SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), id));
496500

497501
if (survey != null)
498502
{
@@ -519,7 +523,7 @@ public ApiResponse execute(SurveyResponseForm form, BindException errors) throws
519523
SurveyDesign surveyDesign = null;
520524

521525
if (form.getSurveyDesignId() != null)
522-
surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), form.getSurveyDesignId());
526+
surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), form.getSurveyDesignId());
523527

524528
if (surveyDesign != null)
525529
{
@@ -733,7 +737,7 @@ public ApiResponse execute(SurveyForm form, BindException errors)
733737

734738
if (survey != null && !survey.isNew())
735739
{
736-
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), survey.getSurveyDesignId());
740+
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), survey.getSurveyDesignId());
737741

738742
if (surveyDesign != null)
739743
{
@@ -857,7 +861,7 @@ public ApiResponse execute(SurveyAttachmentForm form, BindException errors) thro
857861
List<AttachmentFile> files = getAttachmentFileList();
858862
if (!files.isEmpty())
859863
{
860-
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesign(getContainer(), getUser(), survey.getSurveyDesignId());
864+
SurveyDesign surveyDesign = SurveyManager.get().getSurveyDesignForRead(getContainer(), getUser(), survey.getSurveyDesignId());
861865

862866
if (surveyDesign != null)
863867
{

survey/src/org/labkey/survey/SurveyManager.java

Lines changed: 70 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,15 @@
6464
import org.labkey.api.security.User;
6565
import org.labkey.api.security.permissions.AbstractContainerScopingTest;
6666
import org.labkey.api.security.permissions.ReadPermission;
67+
import org.labkey.api.security.roles.AuthorRole;
6768
import org.labkey.api.security.roles.ReaderRole;
6869
import org.labkey.api.survey.model.Survey;
6970
import org.labkey.api.survey.model.SurveyDesign;
7071
import org.labkey.api.survey.model.SurveyListener;
7172
import org.labkey.api.util.JsonUtil;
7273
import org.labkey.api.util.PageFlowUtil;
7374
import org.labkey.api.util.Path;
75+
import org.labkey.api.view.ActionURL;
7476
import org.labkey.api.view.ViewContext;
7577
import org.springframework.validation.BindException;
7678

@@ -268,16 +270,36 @@ public Survey saveSurvey(Container container, User user, Survey survey)
268270
}
269271
}
270272

271-
/** Checks that the user has read permission to the container that owns the design, but we accept cross-container references */
273+
/**
274+
* Checks that the user has read permission to the container that owns the design, but we accept cross-container references
275+
*/
276+
@Nullable
277+
public SurveyDesign getSurveyDesignForRead(Container container, User user, int surveyId)
278+
{
279+
SurveyDesign surveyDesign = _getSurveyDesign(new SimpleFilter(), surveyId);
280+
281+
if (surveyDesign != null)
282+
{
283+
Container actualContainer = ContainerManager.getForId(surveyDesign.getContainerId());
284+
285+
// A survey design can be requested from a different folder provided the user has read permission.
286+
return actualContainer == null || !actualContainer.hasPermission(user, ReadPermission.class) ? null : surveyDesign;
287+
}
288+
return null;
289+
}
290+
291+
@Nullable
292+
public SurveyDesign getSurveyDesignForWrite(Container container, User user, int surveyId)
293+
{
294+
// Container scoping is enforced when updating a survey design.
295+
return _getSurveyDesign(SimpleFilter.createContainerFilter(container), surveyId);
296+
}
297+
272298
@Nullable
273-
public SurveyDesign getSurveyDesign(Container container, User user, int surveyId)
299+
private SurveyDesign _getSurveyDesign(SimpleFilter filter, int surveyId)
274300
{
275-
SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("rowId"), surveyId);
276-
SurveyDesign result = new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class);
277-
if (result == null)
278-
return null;
279-
Container actualContainer = ContainerManager.getForId(result.getContainerId());
280-
return actualContainer == null || !actualContainer.hasPermission(user, ReadPermission.class) ? null : result;
301+
filter.addCondition(FieldKey.fromParts("rowId"), surveyId);
302+
return new TableSelector(SurveySchema.getInstance().getSurveyDesignsTable(), filter, null).getObject(SurveyDesign.class);
281303
}
282304

283305
/**
@@ -462,7 +484,7 @@ public List<Throwable> fireBeforeDeleteSurvey(Container c, User user, Survey sur
462484
public static List<Throwable> fireDeleteSurvey(Container c, User user, Survey survey)
463485
{
464486
List<Throwable> errors = new ArrayList<>();
465-
SurveyDesign design = SurveyManager.get().getSurveyDesign(c, user, survey.getSurveyDesignId());
487+
SurveyDesign design = SurveyManager.get().getSurveyDesignForRead(c, user, survey.getSurveyDesignId());
466488

467489
for (SurveyListener l : _surveyListeners)
468490
{
@@ -825,26 +847,59 @@ public void testSurveyDesignContainerScoping() throws Exception
825847
// 1. Same container: a user with read access in the design's container sees it.
826848
User readerA = createUserInRole(_projectA, ReaderRole.class);
827849
assertNotNull("Design should be visible from its own container to a user with read access",
828-
sm.getSurveyDesign(_projectA, readerA, designId));
850+
sm.getSurveyDesignForRead(_projectA, readerA, designId));
829851

830852
// 2. Different container, caller can read the design's container: tolerated, design is returned.
831853
User readerAB = createUserInRole(_projectA, ReaderRole.class);
832854
grantRole(readerAB, _projectB, ReaderRole.class);
833855
assertNotNull("Design should be visible from another container when the caller can read the design's container",
834-
sm.getSurveyDesign(_projectB, readerAB, designId));
856+
sm.getSurveyDesignForRead(_projectB, readerAB, designId));
835857

836858
// 3. Different container, caller cannot read the design's container: must return null.
837859
User readerB = createUserInRole(_projectB, ReaderRole.class);
838860
assertNull("Design must NOT be visible to a caller without read access to the design's container",
839-
sm.getSurveyDesign(_projectB, readerB, designId));
861+
sm.getSurveyDesignForRead(_projectB, readerB, designId));
840862

841863
// A delete issued from the wrong container must not remove the design
842864
sm.deleteSurveyDesign(_projectB, _user, designId, true);
843-
assertNotNull("Cross-container delete must be a no-op", sm.getSurveyDesign(_projectA, _user, designId));
865+
assertNotNull("Cross-container delete must be a no-op", sm.getSurveyDesignForRead(_projectA, _user, designId));
844866

845867
// A delete from the correct container removes it
846868
sm.deleteSurveyDesign(_projectA, _user, designId, true);
847-
assertNull("Same-container delete should remove the design", sm.getSurveyDesign(_projectA, _user, designId));
869+
assertNull("Same-container delete should remove the design", sm.getSurveyDesignForRead(_projectA, _user, designId));
870+
}
871+
872+
// GH Issue 1308: SaveSurveyTemplateAction must not let a caller in one folder overwrite and reparent a survey
873+
// design owned by another folder.
874+
@Test
875+
public void testSaveSurveyTemplateActionContainerScoping() throws Exception
876+
{
877+
SurveyManager sm = SurveyManager.get();
878+
879+
SurveyDesign design = new SurveyDesign();
880+
design.setLabel("Design owned by A");
881+
design.setDescription("original description");
882+
design = sm.saveSurveyDesign(_projectA, _user, design);
883+
int designId = design.getRowId();
884+
885+
User attacker = createUserInRole(_projectA, ReaderRole.class);
886+
grantRole(attacker, _projectB, AuthorRole.class);
887+
888+
ActionURL url = new ActionURL(SurveyController.SaveSurveyTemplateAction.class, _projectB)
889+
.addParameter("rowId", designId)
890+
.addParameter("label", "STOLEN")
891+
.addParameter("description", "hijacked");
892+
post(url, attacker);
893+
894+
// The design must still belong to folder A with its original field values: not reparented, not overwritten.
895+
SurveyDesign after = sm.getSurveyDesignForRead(_projectA, _user, designId);
896+
assertNotNull("Design must still exist after the cross-container save attempt", after);
897+
assertEquals("Design must NOT be reparented into the attacker's container",
898+
_projectA.getId(), after.getContainerId());
899+
assertEquals("Design label must NOT be overwritten from another container",
900+
"Design owned by A", after.getLabel());
901+
assertEquals("Design description must NOT be overwritten from another container",
902+
"original description", after.getDescription());
848903
}
849904

850905
@Test
@@ -854,7 +909,7 @@ public void testSurveyContainerScoping()
854909

855910
SurveyDesign design = new SurveyDesign();
856911
design.setLabel("Scoping test design for survey");
857-
design = sm.saveSurveyDesign(_projectA, _user, design);
912+
design = sm.saveSurveyDesign(_projectA, _user, design);
858913

859914
Survey survey = new Survey();
860915
survey.setLabel("Scoping test survey");

survey/src/org/labkey/survey/SurveyModule.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ public WebPartView<?> getWebPartView(@NotNull ViewContext context, @NotNull Port
227227
return new HtmlView("Surveys", HtmlString.of("There is no survey design selected to be displayed in this webpart."));
228228
else
229229
{
230-
surveyDesign = SurveyManager.get().getSurveyDesign(context.getContainer(), context.getUser(), Integer.parseInt(designIdStr));
230+
surveyDesign = SurveyManager.get().getSurveyDesignForRead(context.getContainer(), context.getUser(), Integer.parseInt(designIdStr));
231231

232232
if (surveyDesign == null)
233233
return new HtmlView("Surveys", HtmlString.of("The survey design configured for this webpart cannot be found and may have been deleted."));

survey/src/org/labkey/survey/model/SurveyServiceImpl.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public SurveyDesign[] getSurveyDesigns(SimpleFilter filter)
6464
@Override
6565
public SurveyDesign getSurveyDesign(Container container, User user, int surveyDesignId)
6666
{
67-
return SurveyManager.get().getSurveyDesign(container, user, surveyDesignId);
67+
return SurveyManager.get().getSurveyDesignForRead(container, user, surveyDesignId);
6868
}
6969

7070
@Override

0 commit comments

Comments
 (0)