Skip to content

Container scoping for survey save template action. #7887

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
labkey-klum wants to merge 1 commit into
base: release26.7-SNAPSHOT
Choose a base branch
from
+89 −30
Open

Container scoping for survey save template action. #7887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 17 additions & 13 deletions survey/src/org/labkey/survey/SurveyController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand All @@ -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() + ".");
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -340,10 +341,12 @@ public class SaveSurveyTemplateAction extends MutatingApiAction<SurveyDesignForm
public ApiResponse execute(SurveyDesignForm form, BindException errors) throws Exception
{
ApiSimpleResponse response = new ApiSimpleResponse();
SurveyDesign survey = getSurveyDesign(form);
// Updating the survey design. Resolve the design with container scoping.
SurveyDesign survey = getSurveyDesign(form, id -> SurveyManager.get().getSurveyDesignForWrite(getContainer(), getUser(), id));
Map<String, Object> errorInfo = new HashMap<>();

try {
try
{
// try to validate the metadata
String metadata = StringUtils.trimToNull(form.getMetadata());

Expand Down Expand Up @@ -416,21 +419,21 @@ else if (NumberUtils.isDigits(part)) // Should be positive integer
}
}

private SurveyDesign getSurveyDesign(SurveyDesignForm form)
private SurveyDesign getSurveyDesign(SurveyDesignForm form, IntFunction<SurveyDesign> 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");
}
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");
}
Expand Down Expand Up @@ -492,7 +495,8 @@ public class GetSurveyTemplateAction extends ReadOnlyApiAction<SurveyDesignForm>
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)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -857,7 +861,7 @@ public ApiResponse execute(SurveyAttachmentForm form, BindException errors) thro
List<AttachmentFile> 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)
{
Expand Down
85 changes: 70 additions & 15 deletions survey/src/org/labkey/survey/SurveyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,15 @@
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;
import org.labkey.api.survey.model.SurveyListener;
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;

Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -462,7 +484,7 @@ public List<Throwable> fireBeforeDeleteSurvey(Container c, User user, Survey sur
public static List<Throwable> fireDeleteSurvey(Container c, User user, Survey survey)
{
List<Throwable> 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)
{
Expand Down Expand Up @@ -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
Expand All @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion survey/src/org/labkey/survey/SurveyModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."));
Expand Down
2 changes: 1 addition & 1 deletion survey/src/org/labkey/survey/model/SurveyServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down