require browser developer permission in order to create and modify survey designs

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 @@
  *   <li>{@link #createContainer(String)} — make a throwaway child of the junit container (auto-cleaned).</li>
  *   <li>{@link #createUserInRole(Container, Class)} — make a user with a role assigned in <em>one</em> folder only
  *       (auto-cleaned). Use this to obtain a caller who is, say, admin in folder A but has no rights in folder B.</li>
+ *   <li>{@link #grantRootRole(User, Class)} — grant a site-wide role such as Platform Developer (auto-cleaned).</li>
  *   <li>{@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}.</li>
  * </ul>
@@ -66,6 +69,7 @@ public abstract class AbstractContainerScopingTest extends Assert
 
     private final List<Container> _containers = new ArrayList<>();
     private final List<User> _users = new ArrayList<>();
+    private final List<Pair<User, Class<? extends Role>>> _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<? extends Role> 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<? extends Role> 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

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<SurveyDesignForm>
     {
         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<SurveyDesignForm>
     {
         @Override

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<String, Object> getTrimmedMetaData(Map<String, Object> 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()
         {

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());

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<? extends Permission> 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);
     }