diff --git a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java index 543cccbb6e2..ed10a2c7751 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractContainerScopingTest.java @@ -29,7 +29,9 @@ import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; import org.labkey.api.security.roles.Role; +import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.Pair; import org.labkey.api.util.TestContext; import org.labkey.api.view.ActionURL; import org.labkey.api.view.ViewServlet; @@ -52,6 +54,7 @@ *
  • {@link #createContainer(String)} — make a throwaway child of the junit container (auto-cleaned).
    • *
  • {@link #createUserInRole(Container, Class)} — make a user with a role assigned in one folder only * (auto-cleaned). Use this to obtain a caller who is, say, admin in folder A but has no rights in folder B.
    • + *
  • {@link #grantRootRole(User, Class)} — grant a site-wide role such as Platform Developer (auto-cleaned).
    • *
  • {@link #get(ActionURL, User)} / {@link #post(ActionURL, User)} — dispatch an in-JVM request as a given user * and inspect the {@link MockHttpServletResponse} status. Parameters travel on the {@link ActionURL}.
    • * @@ -66,6 +69,7 @@ public abstract class AbstractContainerScopingTest extends Assert private final List _containers = new ArrayList<>(); private final List _users = new ArrayList<>(); + private final List>> _rootRoleGrants = new ArrayList<>(); /** The site-admin user (from {@link TestContext}) that owns the test fixtures. */ protected User getAdmin() @@ -135,6 +139,17 @@ protected void grantRole(User user, Container scope, Class role) SecurityPolicyManager.savePolicyForTests(policy, getAdmin()); } + /** + * Grant {@code role} to {@code user} at the site level, for permissions that are only ever checked against the root + * container (Platform Developer and the other {@code User.isTrusted*} roles). Registered for cleanup: the root + * policy is site-wide, so an assignment left behind would outlive the test. + */ + protected void grantRootRole(User user, Class role) throws Exception + { + grantRole(user, ContainerManager.getRoot(), role); + _rootRoleGrants.add(new Pair<>(user, role)); + } + /** * Dispatch a GET to the action addressed by {@code url} as {@code user}. Put request parameters on the URL. No * request-body Content-Type is sent: a GET carries no body, and an "application/json" Content-Type would make an @@ -169,6 +184,22 @@ public void cleanupContainerScopingFixtures() { User admin = getAdmin(); + if (!_rootRoleGrants.isEmpty()) + { + try + { + MutableSecurityPolicy rootPolicy = new MutableSecurityPolicy(ContainerManager.getRoot().getPolicy()); + // Remove only what grantRootRole added: clearAssignedRoles() would drop every root assignment the + // principal holds, which is site-wide and unrecoverable if the caller passed a pre-existing user. + _rootRoleGrants.forEach(grant -> rootPolicy.removeRoleAssignment(grant.getKey(), RoleManager.getRole(grant.getValue()))); + SecurityPolicyManager.savePolicyForTests(rootPolicy, admin); + } + catch (Exception ignored) + { + } + _rootRoleGrants.clear(); + } + for (User user : _users) { try diff --git a/survey/src/org/labkey/survey/SurveyController.java b/survey/src/org/labkey/survey/SurveyController.java index e4e81d93a27..40fcfac6711 100644 --- a/survey/src/org/labkey/survey/SurveyController.java +++ b/survey/src/org/labkey/survey/SurveyController.java @@ -54,10 +54,12 @@ import org.labkey.api.query.QueryUpdateService; import org.labkey.api.query.QueryView; import org.labkey.api.query.UserSchema; +import org.labkey.api.security.RequiresAllOf; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.permissions.AdminPermission; +import org.labkey.api.security.permissions.BrowserDeveloperPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; @@ -212,7 +214,7 @@ public void addNavTrail(NavTree root) } } - @RequiresPermission(InsertPermission.class) + @RequiresAllOf({InsertPermission.class, BrowserDeveloperPermission.class}) public static class SurveyDesignAction extends SimpleViewAction { private String _title = "Create Survey Design"; @@ -334,7 +336,7 @@ public void setDesignId(String designId) } } - @RequiresPermission(InsertPermission.class) + @RequiresAllOf({InsertPermission.class, BrowserDeveloperPermission.class}) public class SaveSurveyTemplateAction extends MutatingApiAction { @Override diff --git a/survey/src/org/labkey/survey/SurveyManager.java b/survey/src/org/labkey/survey/SurveyManager.java index 1a3795c2a36..1be285d7873 100644 --- a/survey/src/org/labkey/survey/SurveyManager.java +++ b/survey/src/org/labkey/survey/SurveyManager.java @@ -16,6 +16,7 @@ package org.labkey.survey; +import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.collections4.MultiValuedMap; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -50,6 +51,7 @@ import org.labkey.api.data.TableSelector; import org.labkey.api.gwt.client.AuditBehaviorType; import org.labkey.api.module.Module; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.module.ModuleResourceCache; import org.labkey.api.module.ModuleResourceCacheHandler; import org.labkey.api.module.ModuleResourceCaches; @@ -63,8 +65,12 @@ import org.labkey.api.resource.Resource; import org.labkey.api.security.User; import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.permissions.BrowserDeveloperPermission; +import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.security.roles.AuthorRole; +import org.labkey.api.security.roles.PlatformDeveloperRole; import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.survey.model.Survey; import org.labkey.api.survey.model.SurveyDesign; @@ -73,7 +79,9 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; import org.labkey.api.view.ActionURL; +import org.labkey.api.view.UnauthorizedException; import org.labkey.api.view.ViewContext; +import org.labkey.survey.query.SurveyQuerySchema; import org.springframework.validation.BindException; import java.io.IOException; @@ -86,6 +94,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import java.util.function.Function; import java.util.stream.Stream; @@ -210,6 +219,13 @@ public Map getTrimmedMetaData(Map origMap) public SurveyDesign saveSurveyDesign(Container container, User user, SurveyDesign survey) { + // GH Issue 1526: a design's metadata is compiled and run in the viewer's browser. This is the chokepoint every + // caller reaches, including SurveyService; the query update path is gated separately in SurveyDesignTable. + // BrowserDeveloperPermission is a site permission that root role assignments grant in every container, so it + // has to be required alongside the folder-scoped write check, never in place of it. + if (!container.hasPermissions(user, Set.of(InsertPermission.class, BrowserDeveloperPermission.class))) + throw new UnauthorizedException("You must be either a PlatformDeveloper or TrustedAnalyst with insert permission in this folder to create and edit survey designs."); + DbScope scope = SurveySchema.getInstance().getSchema().getScope(); try (DbScope.Transaction transaction = scope.ensureTransaction()) @@ -829,9 +845,12 @@ public static class ContainerScopingTestCase extends AbstractContainerScopingTes @Before public void setUp() { + // Every test here dispatches through SurveyController, and DefaultModule.dispatch 404s before the action + // runs unless the module is active in the container. + Module survey = ModuleLoader.getInstance().getModule("Survey"); _user = getAdmin(); - _projectA = createContainer("A"); - _projectB = createContainer("B"); + _projectA = createContainer("A", survey); + _projectB = createContainer("B", survey); } @Test @@ -884,12 +903,16 @@ public void testSaveSurveyTemplateActionContainerScoping() throws Exception User attacker = createUserInRole(_projectA, ReaderRole.class); grantRole(attacker, _projectB, AuthorRole.class); + // GH Issue 1526 gates the action on BrowserDeveloperPermission, so the attacker needs a developer role + // to reach the container check this test covers. + grantRootRole(attacker, PlatformDeveloperRole.class); ActionURL url = new ActionURL(SurveyController.SaveSurveyTemplateAction.class, _projectB) .addParameter("rowId", designId) .addParameter("label", "STOLEN") .addParameter("description", "hijacked"); - post(url, attacker); + // Container scoping rejects the cross-folder rowId before the design is touched + assertStatus(HttpServletResponse.SC_NOT_FOUND, 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); @@ -902,6 +925,46 @@ public void testSaveSurveyTemplateActionContainerScoping() throws Exception "original description", after.getDescription()); } + // GH Issue 1526: a design's metadata is compiled and run in the viewer's browser, so authoring one requires + // the BrowserDeveloperPermission. Both PlatformDeveloper and TrustedAnalyst are expected to satisfy the check. + @Test + public void testSurveyDesignAuthoringRequiresTrustedAnalyst() throws Exception + { + User author = createUserInRole(_projectA, AuthorRole.class); + assertFalse("Test author must not be a trusted analyst", author.isTrustedAnalyst()); + assertTrue("Site admin is expected to satisfy the trusted analyst check", _user.isTrustedAnalyst()); + + TableInfo designs = QueryService.get() + .getUserSchema(author, _projectA, SurveyQuerySchema.SCHEMA_NAME) + .getTable(SurveyQuerySchema.SURVEY_DESIGN_TABLE_NAME); + assertNotNull("Survey designs table should resolve for an author", designs); + + // The query update path is closed to an untrusted author, so query-insertRows.api cannot reach the metadata column + assertFalse("An untrusted author must not be able to insert a survey design", + designs.hasPermission(author, InsertPermission.class)); + assertFalse("An untrusted author must not be able to update a survey design", + designs.hasPermission(author, UpdatePermission.class)); + // ...but reading the designs grid is unaffected + assertTrue("An author must still be able to read survey designs", + designs.hasPermission(author, ReadPermission.class)); + + // A trusted user keeps both write paths + TableInfo adminDesigns = QueryService.get() + .getUserSchema(_user, _projectA, SurveyQuerySchema.SCHEMA_NAME) + .getTable(SurveyQuerySchema.SURVEY_DESIGN_TABLE_NAME); + assertTrue("A trusted user must be able to insert a survey design", + adminDesigns.hasPermission(_user, InsertPermission.class)); + assertTrue("A trusted user must be able to update a survey design", + adminDesigns.hasPermission(_user, UpdatePermission.class)); + + // The action rejects the same author. UnauthorizedException resolves to 403 rather than 401 for a logged-in user. + ActionURL url = new ActionURL(SurveyController.SaveSurveyTemplateAction.class, _projectA) + .addParameter("label", "Untrusted design") + .addParameter("metadata", "{\"survey\":{\"beforeLoad\":{\"fn\":\"function(){}\"}," + + "\"sections\":[{\"title\":\"s\",\"questions\":[]}]}}"); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, author)); + } + @Test public void testSurveyContainerScoping() { diff --git a/survey/src/org/labkey/survey/query/SurveyDesignQueryView.java b/survey/src/org/labkey/survey/query/SurveyDesignQueryView.java index eed76df2cc7..017cd94179c 100644 --- a/survey/src/org/labkey/survey/query/SurveyDesignQueryView.java +++ b/survey/src/org/labkey/survey/query/SurveyDesignQueryView.java @@ -20,12 +20,15 @@ import org.labkey.api.query.QuerySettings; import org.labkey.api.query.QueryView; import org.labkey.api.query.UserSchema; +import org.labkey.api.security.permissions.BrowserDeveloperPermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.view.ActionURL; import org.labkey.api.view.DataView; import org.labkey.survey.SurveyController; import org.springframework.validation.BindException; +import java.util.Set; + /** * User: klum * Date: 12/10/12 @@ -52,7 +55,7 @@ protected void populateButtonBar(DataView view, ButtonBar bar) { super.populateButtonBar(view, bar); - if (getContainer().hasPermission(getUser(), InsertPermission.class)) + if (getContainer().hasPermissions(getUser(), Set.of(InsertPermission.class, BrowserDeveloperPermission.class))) { ActionURL insertURL = new ActionURL(SurveyController.SurveyDesignAction.class, getContainer()); insertURL.addReturnUrl(getReturnUrl()); diff --git a/survey/src/org/labkey/survey/query/SurveyDesignTable.java b/survey/src/org/labkey/survey/query/SurveyDesignTable.java index bc494f6a1cf..47a50606f04 100644 --- a/survey/src/org/labkey/survey/query/SurveyDesignTable.java +++ b/survey/src/org/labkey/survey/query/SurveyDesignTable.java @@ -26,7 +26,10 @@ import org.labkey.api.query.FilteredTable; import org.labkey.api.query.QueryUpdateService; import org.labkey.api.security.UserPrincipal; +import org.labkey.api.security.permissions.BrowserDeveloperPermission; +import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; +import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.view.ActionURL; import org.labkey.survey.SurveyController; @@ -78,6 +81,12 @@ public QueryUpdateService getUpdateService() @Override public boolean hasPermission(@NotNull UserPrincipal user, @NotNull Class perm) { + // GitHub Issue #1526 treat surveys as executable code. + if (perm.equals(InsertPermission.class) || perm.equals(UpdatePermission.class)) + { + if (!getContainer().hasPermission(user, BrowserDeveloperPermission.class)) + return false; + } return getContainer().hasPermission(user, perm); }