Skip to content

Commit fe3eddb

Browse files
labkey-tchadlabkey-tchad
committed
Merge 25.11 to 26.3
 Conflicts: ehr_billing/src/org/labkey/ehr_billing/EHR_BillingManager.java
2 parents c3a4705 + 23ddde0 commit fe3eddb

5 files changed

Lines changed: 419 additions & 6 deletions

File tree

‎ehr/src/org/labkey/ehr/EHRController.java‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,21 @@
1616

1717
package org.labkey.ehr;
1818

19+
import jakarta.servlet.http.HttpServletRequest;
1920
import org.apache.commons.lang3.StringUtils;
2021
import org.apache.logging.log4j.Logger;
2122
import org.jetbrains.annotations.NotNull;
23+
import org.junit.AfterClass;
24+
import org.junit.Assert;
25+
import org.junit.BeforeClass;
26+
import org.junit.Test;
2227
import org.json.JSONArray;
2328
import org.json.JSONObject;
2429
import org.labkey.api.action.ApiResponse;
2530
import org.labkey.api.action.ApiSimpleResponse;
2631
import org.labkey.api.action.ConfirmAction;
2732
import org.labkey.api.action.MutatingApiAction;
33+
import org.labkey.api.action.NullSafeBindException;
2834
import org.labkey.api.action.ReadOnlyApiAction;
2935
import org.labkey.api.action.SimpleErrorView;
3036
import org.labkey.api.action.SimpleViewAction;
@@ -38,6 +44,9 @@
3844
import org.labkey.api.data.DbScope;
3945
import org.labkey.api.data.DisplayColumn;
4046
import org.labkey.api.data.JsonWriter;
47+
import org.labkey.api.data.NormalContainerType;
48+
import org.labkey.api.data.PropertyManager;
49+
import org.labkey.api.data.PropertyManager.WritablePropertyMap;
4150
import org.labkey.api.data.RuntimeSQLException;
4251
import org.labkey.api.data.SimpleFilter;
4352
import org.labkey.api.data.Sort;
@@ -78,17 +87,27 @@
7887
import org.labkey.api.reader.TabLoader;
7988
import org.labkey.api.resource.FileResource;
8089
import org.labkey.api.resource.Resource;
90+
import org.labkey.api.security.MutableSecurityPolicy;
8191
import org.labkey.api.security.RequiresPermission;
92+
import org.labkey.api.security.SecurityManager;
93+
import org.labkey.api.security.SecurityPolicyManager;
94+
import org.labkey.api.security.User;
95+
import org.labkey.api.security.UserManager;
96+
import org.labkey.api.security.ValidEmail;
8297
import org.labkey.api.security.permissions.AdminPermission;
8398
import org.labkey.api.security.permissions.DeletePermission;
8499
import org.labkey.api.security.permissions.ReadPermission;
85100
import org.labkey.api.security.permissions.UpdatePermission;
101+
import org.labkey.api.security.roles.FolderAdminRole;
86102
import org.labkey.api.settings.AppProps;
87103
import org.labkey.api.study.DatasetTable;
88104
import org.labkey.api.util.ExceptionUtil;
105+
import org.labkey.api.util.GUID;
89106
import org.labkey.api.util.HtmlStringBuilder;
107+
import org.labkey.api.util.JunitUtil;
90108
import org.labkey.api.util.PageFlowUtil;
91109
import org.labkey.api.util.Path;
110+
import org.labkey.api.util.TestContext;
92111
import org.labkey.api.util.URLHelper;
93112
import org.labkey.api.util.logging.LogHelper;
94113
import org.labkey.api.view.ActionURL;
@@ -97,6 +116,7 @@
97116
import org.labkey.api.view.NavTree;
98117
import org.labkey.api.view.Portal;
99118
import org.labkey.api.view.UnauthorizedException;
119+
import org.labkey.api.view.ViewContext;
100120
import org.labkey.api.view.WebPartFactory;
101121
import org.labkey.api.view.WebPartView;
102122
import org.labkey.api.view.template.ClientDependency;
@@ -722,6 +742,16 @@ public void setIds(String[] ids)
722742
}
723743
}
724744

745+
/**
746+
* SECURITY — ELEVATED ACCESS BY DESIGN:
747+
* Demographics are served from EHRDemographicsService, whose cache is intentionally built under the configured
748+
* EHR service user (EHRService.getEHRUser(c)), NOT the requesting user. Callers therefore receive aggregated
749+
* demographic/derived fields regardless of their per-dataset / per-QCState row permissions. This is the intended
750+
* EHR design: Read access to an EHR study folder is the trust gate for the demographics summary, which backs the
751+
* core data-entry and animal-history UIs via the shared EHR.DemographicsCache client. See
752+
* {@link org.labkey.ehr.demographics.EHRDemographicsServiceImpl#getAnimals}. Do not expose the returned record map
753+
* to a lower trust boundary without re-securing it.
754+
*/
725755
@RequiresPermission(ReadPermission.class)
726756
public static class GetDemographicsAction extends ReadOnlyApiAction<GetDemographicsForm>
727757
{
@@ -773,6 +803,13 @@ public ApiResponse execute(ScheduleGeneticCalculationForm form, BindException er
773803
errors.reject(ERROR_MSG, "Unable to find container for path: " + form.getContainerPath());
774804
return null;
775805
}
806+
807+
// The @RequiresPermission check only covers the request container; re-verify admin on the
808+
// resolved target so a folder admin can't redirect the (global) genetic-calc job at a
809+
// container they don't administer.
810+
if (!c.hasPermission(getUser(), AdminPermission.class))
811+
throw new UnauthorizedException("You do not have permission to configure genetic calculations for container: " + c.getPath());
812+
776813
GeneticCalculationsJob.setProperties(form.isEnabled(), c, form.getHourOfDay(), form.getDayOfWeek(), form.isKinshipValidation(), form.isAllowImportDuringBusinessHours());
777814

778815
return new ApiSimpleResponse("success", true);
@@ -2570,4 +2607,144 @@ public void setStartingId(Integer startingId)
25702607
_startingId = startingId;
25712608
}
25722609
}
2610+
2611+
/**
2612+
* Verifies the cross-container authorization of {@link SetGeneticCalculationTaskSettingsAction}. The
2613+
* {@code @RequiresPermission(AdminPermission.class)} annotation only guards the request container, but the action
2614+
* resolves and acts on a caller-supplied {@code containerPath}. This test confirms a user who administers the
2615+
* request container but not the resolved target container is rejected, while a user who administers the target
2616+
* succeeds.
2617+
*/
2618+
public static class TestCase extends Assert
2619+
{
2620+
private static final String REQUEST_ADMIN_EMAIL = "[email protected]";
2621+
private static final String BOTH_ADMIN_EMAIL = "[email protected]";
2622+
2623+
private static Container _requestContainer;
2624+
private static Container _targetContainer;
2625+
private static User _requestAdmin; // folder admin on the request container only
2626+
private static User _bothAdmin; // folder admin on both the request and target containers
2627+
2628+
@BeforeClass
2629+
public static void setup() throws Exception
2630+
{
2631+
cleanup();
2632+
2633+
User testUser = TestContext.get().getUser();
2634+
Container junit = JunitUtil.getTestContainer();
2635+
_requestContainer = ContainerManager.createContainer(junit, "EHRGeneticCalcTest-request-" + GUID.makeGUID(), null, null, NormalContainerType.NAME, testUser);
2636+
_targetContainer = ContainerManager.createContainer(junit, "EHRGeneticCalcTest-target-" + GUID.makeGUID(), null, null, NormalContainerType.NAME, testUser);
2637+
2638+
_requestAdmin = SecurityManager.addUser(new ValidEmail(REQUEST_ADMIN_EMAIL), null).getUser();
2639+
_bothAdmin = SecurityManager.addUser(new ValidEmail(BOTH_ADMIN_EMAIL), null).getUser();
2640+
2641+
MutableSecurityPolicy requestPolicy = new MutableSecurityPolicy(_requestContainer, _requestContainer.getPolicy());
2642+
requestPolicy.addRoleAssignment(_requestAdmin, FolderAdminRole.class);
2643+
requestPolicy.addRoleAssignment(_bothAdmin, FolderAdminRole.class);
2644+
SecurityPolicyManager.savePolicyForTests(requestPolicy, testUser);
2645+
2646+
MutableSecurityPolicy targetPolicy = new MutableSecurityPolicy(_targetContainer, _targetContainer.getPolicy());
2647+
targetPolicy.addRoleAssignment(_bothAdmin, FolderAdminRole.class);
2648+
SecurityPolicyManager.savePolicyForTests(targetPolicy, testUser);
2649+
}
2650+
2651+
@Test
2652+
public void testCannotConfigureForeignContainer()
2653+
{
2654+
// _requestAdmin administers the request container but NOT the target container, so pointing the
2655+
// (global) genetic-calc schedule at the target container must be rejected.
2656+
SetGeneticCalculationTaskSettingsAction action = new SetGeneticCalculationTaskSettingsAction();
2657+
action.setViewContext(makeContext(_requestAdmin, _requestContainer));
2658+
2659+
ScheduleGeneticCalculationForm form = new ScheduleGeneticCalculationForm();
2660+
form.setEnabled(false);
2661+
form.setContainerPath(_targetContainer.getPath());
2662+
form.setHourOfDay(2);
2663+
2664+
try
2665+
{
2666+
action.execute(form, new NullSafeBindException(form, "form"));
2667+
fail("Expected UnauthorizedException when targeting a container the user does not administer");
2668+
}
2669+
catch (UnauthorizedException expected)
2670+
{
2671+
// expected
2672+
}
2673+
}
2674+
2675+
@Test
2676+
public void testCanConfigureAdministeredContainer()
2677+
{
2678+
// _bothAdmin administers the target container, so configuring the schedule for it must succeed. Preserve
2679+
// and restore the server-global settings this action mutates so the test leaves no side effects.
2680+
Map<String, String> original = new HashMap<>(PropertyManager.getProperties(GeneticCalculationsJob.GENETICCALCULATIONS_PROPERTY_DOMAIN));
2681+
try
2682+
{
2683+
SetGeneticCalculationTaskSettingsAction action = new SetGeneticCalculationTaskSettingsAction();
2684+
action.setViewContext(makeContext(_bothAdmin, _requestContainer));
2685+
2686+
ScheduleGeneticCalculationForm form = new ScheduleGeneticCalculationForm();
2687+
form.setEnabled(false); // keep disabled so no Quartz job is actually scheduled
2688+
form.setContainerPath(_targetContainer.getPath());
2689+
form.setHourOfDay(2);
2690+
2691+
ApiResponse response = action.execute(form, new NullSafeBindException(form, "form"));
2692+
assertNotNull("Expected a response when configuring an administered container", response);
2693+
assertNotNull("Expected the target container to be persisted", GeneticCalculationsJob.getContainer());
2694+
assertEquals("Schedule should target the requested container", _targetContainer.getId(), GeneticCalculationsJob.getContainer().getId());
2695+
}
2696+
finally
2697+
{
2698+
WritablePropertyMap pm = PropertyManager.getWritableProperties(GeneticCalculationsJob.GENETICCALCULATIONS_PROPERTY_DOMAIN, true);
2699+
pm.clear();
2700+
pm.putAll(original);
2701+
pm.save();
2702+
// Resync the live scheduler to the restored settings.
2703+
GeneticCalculationsJob.unschedule();
2704+
GeneticCalculationsJob.schedule();
2705+
}
2706+
}
2707+
2708+
private ViewContext makeContext(User u, Container c)
2709+
{
2710+
HttpServletRequest request = TestContext.get().getRequest();
2711+
ViewContext context = new ViewContext();
2712+
context.setContainer(c);
2713+
context.setUser(u);
2714+
context.setRequest(request);
2715+
return context;
2716+
}
2717+
2718+
@AfterClass
2719+
public static void cleanup()
2720+
{
2721+
User testUser = TestContext.get().getUser();
2722+
2723+
if (_targetContainer != null)
2724+
{
2725+
ContainerManager.delete(_targetContainer, testUser);
2726+
_targetContainer = null;
2727+
}
2728+
if (_requestContainer != null)
2729+
{
2730+
ContainerManager.delete(_requestContainer, testUser);
2731+
_requestContainer = null;
2732+
}
2733+
2734+
for (String email : new String[]{REQUEST_ADMIN_EMAIL, BOTH_ADMIN_EMAIL})
2735+
{
2736+
try
2737+
{
2738+
User u = UserManager.getUser(new ValidEmail(email));
2739+
if (u != null)
2740+
UserManager.deleteUser(u.getUserId());
2741+
}
2742+
catch (ValidEmail.InvalidEmailException | SecurityManager.UserManagementException ignored)
2743+
{
2744+
}
2745+
}
2746+
_requestAdmin = null;
2747+
_bothAdmin = null;
2748+
}
2749+
}
25732750
}

‎ehr/src/org/labkey/ehr/EHRModule.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,12 @@ public String getName()
139139
return 26.003;
140140
}
141141

142+
@Override
143+
public @NotNull Set<Class<?>> getIntegrationTests()
144+
{
145+
return Set.of(EHRController.TestCase.class);
146+
}
147+
142148
@Override
143149
protected void init()
144150
{

‎ehr/src/org/labkey/ehr/demographics/EHRDemographicsServiceImpl.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,16 @@ public AnimalRecord getAnimal(Container c, String id)
118118

119119
/**
120120
* Queries the cache for the animal record, creating if not found.
121-
* Always returns a copy of the original
121+
* Always returns a copy of the original.
122+
*
123+
* <p>SECURITY &mdash; ELEVATED ACCESS BY DESIGN: records are built and cached under the configured EHR service
124+
* user ({@link EHRService#getEHRUser(Container)}), NOT the calling user, and are shared across all users in the
125+
* container. The aggregated demographic/derived fields therefore reflect the EHR service user's access, bypassing
126+
* the caller's per-dataset / per-QCState row permissions (note that many {@code DemographicsProvider}s set
127+
* {@code _supportsQCState = false}, so no QCState filter is applied at all). This is intentional: within an EHR
128+
* study folder, Read access is the trust gate for the demographics summary. Callers exposing these records to an
129+
* end user must treat folder Read as the boundary and must NOT forward them to a lower trust boundary without
130+
* re-securing the data against the consuming user.
122131
*/
123132
@Override
124133
public List<AnimalRecord> getAnimals(Container c, Collection<String> ids)

0 commit comments

Comments
 (0)