diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index cd6765adee2..d613909b2b6 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -135,6 +135,7 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.UserManager; import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.impersonation.ImpersonationTestCase; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.AppPropsTestCase; import org.labkey.api.settings.BaseServerProperties; @@ -398,6 +399,7 @@ public void registerServlets(ServletContext servletCtx) FileUtil.TestCase.class, GenerateUniqueDataIterator.TestCase.class, HelpTopic.TestCase.class, + ImpersonationTestCase.class, InlineInClauseGenerator.TestCase.class, JSONDataLoader.HeaderMatchTest.class, JSONDataLoader.MetadataTest.class, diff --git a/api/src/org/labkey/api/admin/AdminUrls.java b/api/src/org/labkey/api/admin/AdminUrls.java index 4f2e30826d5..b22cefd042f 100644 --- a/api/src/org/labkey/api/admin/AdminUrls.java +++ b/api/src/org/labkey/api/admin/AdminUrls.java @@ -65,7 +65,9 @@ public interface AdminUrls extends UrlProvider ActionURL getSessionLoggingURL(); ActionURL getTrackedAllocationsViewerURL(); ActionURL getSystemMaintenanceURL(); - ActionURL getCspReportToURL(String cspVersion); + ActionURL getCspReportToURL(); + + ActionURL getAllowedExternalRedirectHostsURL(); /** * Simply adds an "Admin Console" link to nav trail if invoked in the root container. Otherwise, root is unchanged. diff --git a/api/src/org/labkey/api/audit/query/DefaultAuditTypeTable.java b/api/src/org/labkey/api/audit/query/DefaultAuditTypeTable.java index 4a1647ae863..3f3b0af48a2 100644 --- a/api/src/org/labkey/api/audit/query/DefaultAuditTypeTable.java +++ b/api/src/org/labkey/api/audit/query/DefaultAuditTypeTable.java @@ -119,6 +119,11 @@ protected void initColumns() @Override protected SimpleFilter.FilterClause getContainerFilterClause(ContainerFilter filter, FieldKey fieldKey) { + // TODO: Setting a contextual role on the container filter clause should not be necessary; the user passed + // (separately) to the ContainerFilter should have the appropriate permission. However, some app actions + // (GetTransactionRowIdsAction, maybe GetLocationHistoryAction, etc.) have been relying on this behavior. Clean + // this up soon, but not for 26.3. Note that this is the only code path that passes contextual roles into + // createFilterClause(), so we could eliminate that option during clean up. User user = (null == getUserSchema()) ? null : getUserSchema().getUser(); Set roles = SecurityManager.canSeeAuditLog(user) ? RoleManager.roleSet(CanSeeAuditLogRole.class) : null; return filter.createFilterClause(getSchema(), fieldKey, CanSeeAuditLogPermission.class, roles); diff --git a/api/src/org/labkey/api/data/Container.java b/api/src/org/labkey/api/data/Container.java index 0d5186156a7..0a397c0340d 100644 --- a/api/src/org/labkey/api/data/Container.java +++ b/api/src/org/labkey/api/data/Container.java @@ -20,6 +20,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Strings; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -827,7 +828,7 @@ public static boolean isLegalName(String name, boolean isProject, StringBuilder return false; } - if (StringUtils.endsWithIgnoreCase(name, ".view") || StringUtils.endsWithIgnoreCase(name, ".api") || StringUtils.endsWithIgnoreCase(name, ".post")) + if (Strings.CI.endsWith(name, ".view") || Strings.CI.endsWith(name, ".api") || Strings.CI.endsWith(name, ".post")) { error.append("Folder name should not end with '.view', '.api', or '.post'."); return false; @@ -1249,7 +1250,7 @@ public boolean getAsBoolean() } // always put the required modules in the set - // note that this will pickup the modules from the folder type's getActiveModules() + // note that this will pick up the modules from the folder type's getActiveModules() Set modules = new HashSet<>(getRequiredModules()); // add all modules found in user preferences: diff --git a/api/src/org/labkey/api/data/ContainerFilter.java b/api/src/org/labkey/api/data/ContainerFilter.java index 4576d8ec25a..fb495327f14 100644 --- a/api/src/org/labkey/api/data/ContainerFilter.java +++ b/api/src/org/labkey/api/data/ContainerFilter.java @@ -169,9 +169,9 @@ public SimpleFilter.FilterClause createFilterClause(DbSchema schema, FieldKey co } /** Create a FilterClause that restricts based on the containers that meet the filter and user that meets the permission*/ - public SimpleFilter.FilterClause createFilterClause(DbSchema schema, FieldKey containerFilterColumn, Class permission, Set roles) + public SimpleFilter.FilterClause createFilterClause(DbSchema schema, FieldKey containerFilterColumn, Class permission, Set contextualRoles) { - return new ContainerClause(schema, containerFilterColumn, this, permission, roles); + return new ContainerClause(schema, containerFilterColumn, this, permission, contextualRoles); } diff --git a/api/src/org/labkey/api/data/RecordFactory.java b/api/src/org/labkey/api/data/RecordFactory.java index abd5470f2b0..4b4c5a201c6 100644 --- a/api/src/org/labkey/api/data/RecordFactory.java +++ b/api/src/org/labkey/api/data/RecordFactory.java @@ -52,7 +52,7 @@ public RecordFactory(Class clazz) { Object[] params = Arrays.stream(_parameters).map(p -> { Object value = m.get(p.getName()); - return ConvertUtils.convert(value, p.getType()); + return value != null ? ConvertUtils.convert(value, p.getType()) : null; }).toArray(); try diff --git a/api/src/org/labkey/api/data/SqlScriptExecutor.java b/api/src/org/labkey/api/data/SqlScriptExecutor.java index 756b29891b0..e8df99cc377 100644 --- a/api/src/org/labkey/api/data/SqlScriptExecutor.java +++ b/api/src/org/labkey/api/data/SqlScriptExecutor.java @@ -221,6 +221,7 @@ public void execute() { LOG.info("Executing {}", upgradeMethod.getDisplayName()); _moduleContext.invokeUpgradeMethod(upgradeMethod); + LOG.info("Finished executing {}", upgradeMethod.getDisplayName()); } } catch (NoSuchMethodException e) diff --git a/api/src/org/labkey/api/data/TempTableTracker.java b/api/src/org/labkey/api/data/TempTableTracker.java index d62b76b4c31..919b96633d3 100644 --- a/api/src/org/labkey/api/data/TempTableTracker.java +++ b/api/src/org/labkey/api/data/TempTableTracker.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.io.RandomAccessFile; import java.lang.ref.Cleaner; +import java.util.ArrayList; import java.util.Map; import java.util.TreeMap; import java.util.TreeSet; @@ -286,7 +287,8 @@ public void shutdownStarted() { synchronized(createdTableNames) { - for (TempTableTracker ttt : createdTableNames.values()) + // Copy createdTableNames.values() to prevent ConcurrentModificationException + for (TempTableTracker ttt : new ArrayList<>(createdTableNames.values())) { ttt.state.run(); } diff --git a/api/src/org/labkey/api/data/dialect/PostgreSqlService.java b/api/src/org/labkey/api/data/dialect/PostgreSqlService.java new file mode 100644 index 00000000000..fec37101cbc --- /dev/null +++ b/api/src/org/labkey/api/data/dialect/PostgreSqlService.java @@ -0,0 +1,18 @@ +package org.labkey.api.data.dialect; + +import org.labkey.api.services.ServiceRegistry; + +public interface PostgreSqlService +{ + static PostgreSqlService get() + { + return ServiceRegistry.get().getService(PostgreSqlService.class); + } + + static void setInstance(PostgreSqlService impl) + { + ServiceRegistry.get().registerService(PostgreSqlService.class, impl); + } + + BasePostgreSqlDialect getDialect(); +} diff --git a/api/src/org/labkey/api/exp/api/ExpProtocol.java b/api/src/org/labkey/api/exp/api/ExpProtocol.java index eb633711b42..41d5b91af3f 100644 --- a/api/src/org/labkey/api/exp/api/ExpProtocol.java +++ b/api/src/org/labkey/api/exp/api/ExpProtocol.java @@ -99,7 +99,7 @@ enum Status String getContact(); List getChildProtocols(); - List getBatches(); + List getBatches(@Nullable Container c); void setEntityId(String entityId); String getEntityId(); diff --git a/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java b/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java index 811d3375cf0..971d12f6c2c 100644 --- a/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java +++ b/api/src/org/labkey/api/module/IgnoresForbiddenProjectCheck.java @@ -7,7 +7,7 @@ /** * Designates actions that should not enforce forbidden project checking, which is used to support impersonation * container restrictions and project locking. Use with caution. Allows project admins to perform actions outside - * of the project they administer and non-admins the ability to invoke actions in locked projects. + * the project they administer and non-admins the ability to invoke actions in locked projects. * * @see org.labkey.api.action.PermissionCheckableAction#checkPermissions() * @see org.labkey.api.data.Container#isForbiddenProject(org.labkey.api.security.User) diff --git a/api/src/org/labkey/api/qc/TsvDataSerializer.java b/api/src/org/labkey/api/qc/TsvDataSerializer.java index 391d3aaa376..839ef801867 100644 --- a/api/src/org/labkey/api/qc/TsvDataSerializer.java +++ b/api/src/org/labkey/api/qc/TsvDataSerializer.java @@ -91,8 +91,8 @@ private List exportData(DataIteratorBuilder data, List columns, sep = "\t"; } pw.println(); - writeRow(row, columns, pw, tsvWriter); } + writeRow(row, columns, pw, tsvWriter); // GitHub Issue #875: write the first row regardless of whether we had a header or not // write the remaining rows while (iter.next()) diff --git a/api/src/org/labkey/api/query/QueryView.java b/api/src/org/labkey/api/query/QueryView.java index be99e20907c..bdf1e5c638e 100644 --- a/api/src/org/labkey/api/query/QueryView.java +++ b/api/src/org/labkey/api/query/QueryView.java @@ -1050,52 +1050,6 @@ public ActionButton createDeleteButton(boolean showConfirmation) return null; } - public ActionButton createDeleteAllRowsButton(String tableNoun) - { - ActionButton deleteAllRows = new ActionButton("Delete All Rows"); - deleteAllRows.setDisplayPermission(AdminPermission.class); - deleteAllRows.setActionType(ActionButton.Action.SCRIPT); - deleteAllRows.setScript( - "LABKEY.requiresExt4Sandbox(function() {" + - "Ext4.Msg.confirm('Confirm Deletion', 'Are you sure you wish to delete all rows in this " + tableNoun + "? This action cannot be undone and will result in an empty " + tableNoun + ".', function(button){" + - "if (button == 'yes'){" + - "var waitMask = Ext4.Msg.wait('Deleting Rows...', 'Delete Rows'); " + - "Ext4.Ajax.request({ " + - "url : LABKEY.ActionURL.buildURL('query', 'truncateTable'), " + - "method : 'POST', " + - "success: function(response) " + - "{" + - "waitMask.close(); " + - "var data = Ext4.JSON.decode(response.responseText); " + - "Ext4.Msg.show({ " + - "title : 'Success', " + - "buttons : Ext4.MessageBox.OK, " + - "msg : data.deletedRows + ' rows deleted', " + - "fn: function(btn) " + - "{ " + - "if(btn == 'ok') " + - "{ " + - "window.location.reload(); " + - "} " + - "} " + - "})" + - "}, " + - "failure : function(response, opts) " + - "{ " + - "waitMask.close(); " + - "Ext4.getBody().unmask(); " + - "LABKEY.Utils.displayAjaxErrorResponse(response, opts); " + - "}, " + - "jsonData : {schemaName : " + PageFlowUtil.jsString(getQueryDef().getSchema().getName()) + ", queryName : " + PageFlowUtil.jsString(getQueryDef().getName()) + "}, " + - "scope : this " + - "});" + - "}" + - "});" + - "});" - ); - return deleteAllRows; - } - public ActionButton createInsertMenuButton() { return createInsertMenuButton(null, null); diff --git a/api/src/org/labkey/api/reports/ReportService.java b/api/src/org/labkey/api/reports/ReportService.java index f246d67da72..3fdba71398b 100644 --- a/api/src/org/labkey/api/reports/ReportService.java +++ b/api/src/org/labkey/api/reports/ReportService.java @@ -47,6 +47,8 @@ public interface ReportService { + String R_REPORT_CUSTOM_SHARING = "rReportCustomSharing"; + // this logger is to enable all report loggers in the admin ui (org.labkey.api.reports.*) @SuppressWarnings({"UnusedDeclaration", "SSBasedInspection"}) Logger packageLogger = LogManager.getLogger(ReportService.class.getPackageName()); diff --git a/api/src/org/labkey/api/reports/report/r/RReport.java b/api/src/org/labkey/api/reports/report/r/RReport.java index be6e61a388a..83371fcb9b7 100644 --- a/api/src/org/labkey/api/reports/report/r/RReport.java +++ b/api/src/org/labkey/api/reports/report/r/RReport.java @@ -46,6 +46,7 @@ import org.labkey.api.rstudio.RStudioService; import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; +import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.thumbnail.Thumbnail; import org.labkey.api.util.FileUtil; import org.labkey.api.util.PageFlowUtil; @@ -74,6 +75,8 @@ import java.util.Map; import java.util.Set; +import static org.labkey.api.reports.ReportService.R_REPORT_CUSTOM_SHARING; + public class RReport extends ExternalScriptEngineReport { public static final String TYPE = "ReportService.rReport"; @@ -925,8 +928,12 @@ public String getEditAreaSyntax() @Override public boolean allowShareButton(User user, Container container) { - // allow sharing if this R report is a DB report and the user canShare - return !getDescriptor().isModuleBased() && canShare(user, container); + if (OptionalFeatureService.get().isFeatureEnabled(R_REPORT_CUSTOM_SHARING)) + { + // allow sharing if this R report is a DB report and the user canShare + return !getDescriptor().isModuleBased() && canShare(user, container); + } + return false; } public static class TestCase extends Assert diff --git a/api/src/org/labkey/api/security/NormalPermissionsContext.java b/api/src/org/labkey/api/security/NormalPermissionsContext.java index c5e7dc59115..7b987fca457 100644 --- a/api/src/org/labkey/api/security/NormalPermissionsContext.java +++ b/api/src/org/labkey/api/security/NormalPermissionsContext.java @@ -22,8 +22,7 @@ import org.labkey.api.security.impersonation.ImpersonationContextFactory; import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; import org.labkey.api.security.impersonation.UserImpersonationContextFactory; -import org.labkey.api.security.permissions.AdminPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NavTree; @@ -93,8 +92,9 @@ public void addMenu(NavTree menu, Container c, User user, ActionURL currentURL) { @Nullable Container project = c.getProject(); - // Must be site or project admin (folder admins can't impersonate) - if (user.hasRootAdminPermission() || (null != project && project.hasPermission(user, AdminPermission.class))) + // Site admin, app admin, and impersonating troubleshooter can impersonate anywhere; project admin can + // impersonate in that project. Folder admins can't impersonate. + if (user.hasRootPermission(ImpersonatePermission.class) || (project != null && project.hasPermission(user, ImpersonatePermission.class))) { NavTree impersonateMenu = new NavTree("Impersonate"); UserImpersonationContextFactory.addMenu(impersonateMenu); @@ -102,13 +102,6 @@ public void addMenu(NavTree menu, Container c, User user, ActionURL currentURL) RoleImpersonationContextFactory.addMenu(impersonateMenu); menu.addChild(impersonateMenu); } - // Or Impersonating Troubleshooter to impersonate site roles only - else if (null == project && user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class)) - { - NavTree impersonateMenu = new NavTree("Impersonate"); - RoleImpersonationContextFactory.addMenu(impersonateMenu); - menu.addChild(impersonateMenu); - } } NavTree signOut = new NavTree("Sign Out", PageFlowUtil.urlProvider(LoginUrls.class).getLogoutURL(c)); diff --git a/api/src/org/labkey/api/security/PermissionsContext.java b/api/src/org/labkey/api/security/PermissionsContext.java index 16672fd7081..bfd321c41ac 100644 --- a/api/src/org/labkey/api/security/PermissionsContext.java +++ b/api/src/org/labkey/api/security/PermissionsContext.java @@ -64,7 +64,7 @@ default Stream getAssignedRoles(User user, SecurableResource resource) if (!(role instanceof AbstractRootContainerRole siteRole)) throw new IllegalStateException("Root roles should all be AbstractRootContainerRole"); - return siteRole.isAvailableEverywhere() || resource.equals(root); + return siteRole.isApplicableOutsideRoot() || resource.equals(root); }); if (!resource.equals(root)) diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 9eb00cbfa75..e6ccfc41323 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -74,7 +74,7 @@ import org.labkey.api.security.permissions.AbstractPermission; import org.labkey.api.security.permissions.AddUserPermission; import org.labkey.api.security.permissions.AdminPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; @@ -822,7 +822,7 @@ public static void impersonateUser(ViewContext viewContext, User impersonatedUse @Nullable Container project = viewContext.getContainer().getProject(); User user = viewContext.getUser(); - if (user.hasRootAdminPermission()) + if (user.hasRootPermission(ImpersonatePermission.class)) project = null; impersonate(viewContext, new UserImpersonationContextFactory(project, user, impersonatedUser, returnUrl)); @@ -839,7 +839,7 @@ public static void impersonateRoles(ViewContext viewContext, Collection ne @Nullable Container project = viewContext.getContainer().getProject(); User user = viewContext.getUser(); - if (user.hasRootPermission(CanImpersonateSiteRolesPermission.class)) + if (user.hasRootPermission(ImpersonatePermission.class)) project = null; impersonate(viewContext, new RoleImpersonationContextFactory(project, user, newImpersonationRoles, currentImpersonationRoles, returnUrl)); @@ -1412,7 +1412,7 @@ public static void ensureAtLeastOneRootAdminExists() } // A permission class that uniquely identifies the root admins, of which we insist there must be at least one - public static final Class ROOT_ADMIN_PERMISSION = CanImpersonateSiteRolesPermission.class; + public static final Class ROOT_ADMIN_PERMISSION = ImpersonatePermission.class; public static boolean isRootAdmin(User user) { diff --git a/api/src/org/labkey/api/security/User.java b/api/src/org/labkey/api/security/User.java index 2d28b8263d2..70849d2320f 100644 --- a/api/src/org/labkey/api/security/User.java +++ b/api/src/org/labkey/api/security/User.java @@ -33,7 +33,7 @@ import org.labkey.api.security.permissions.AnalystPermission; import org.labkey.api.security.permissions.ApplicationAdminPermission; import org.labkey.api.security.permissions.BrowserDeveloperPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; @@ -594,7 +594,7 @@ public static JSONObject getUserProps(User user, User currentUser, @Nullable Con props.put("isAdmin", nonNullContainer && container.hasPermission(user, AdminPermission.class)); props.put("isRootAdmin", user.hasRootAdminPermission()); props.put("isSystemAdmin", user.hasSiteAdminPermission()); - props.put("canImpersonateSiteRoles", user.hasRootPermission(CanImpersonateSiteRolesPermission.class)); + props.put("canImpersonateSiteRoles", user.hasRootPermission(ImpersonatePermission.class)); props.put("isGuest", user.isGuest()); props.put("isDeveloper", user.isBrowserDev()); props.put("isAnalyst", user.hasRootPermission(AnalystPermission.class)); diff --git a/api/src/org/labkey/api/security/UserCache.java b/api/src/org/labkey/api/security/UserCache.java index f6dbd6bb6f7..72ff900595d 100644 --- a/api/src/org/labkey/api/security/UserCache.java +++ b/api/src/org/labkey/api/security/UserCache.java @@ -83,8 +83,8 @@ private UserCache() return null != user ? user.cloneUser() : null; } - // Returns a deep copy of the active users list, allowing callers to interrogate user permissions without affecting - // cached users. Collection is ordered by email... maybe it should be ordered by display name? + // Returns a mutable deep copy of the active users list, allowing callers to interrogate user permissions without + // affecting cached users. Collection is ordered by email... maybe it should be ordered by display name? static @NotNull Collection getActiveUsers() { List activeUsers = getUserCollections().getActiveUsers(); @@ -95,7 +95,7 @@ private UserCache() .collect(Collectors.toCollection(LinkedList::new)); } - // Returns a deep copy of the users list including deactivated accounts, allowing callers to interrogate user permissions + // Returns a mutable deep copy of the users list including deactivated accounts, allowing callers to interrogate user permissions // without affecting cached users. Collection is ordered randomly... maybe it should be ordered by display name? static @NotNull Collection getActiveAndInactiveUsers() { @@ -104,7 +104,7 @@ private UserCache() return users .stream() .map(User::cloneUser) - .collect(Collectors.toList()); + .collect(Collectors.toCollection(LinkedList::new)); } static @NotNull List getUserIds() @@ -114,7 +114,7 @@ private UserCache() static @NotNull Map getUserEmailMap() { - return Collections.unmodifiableMap(getUserCollections().getEmailMap()); + return getUserCollections().getEmailMap(); } static int getActiveUserCount() diff --git a/api/src/org/labkey/api/security/UserManager.java b/api/src/org/labkey/api/security/UserManager.java index b3bb5c15105..2d3aa600e01 100644 --- a/api/src/org/labkey/api/security/UserManager.java +++ b/api/src/org/labkey/api/security/UserManager.java @@ -59,7 +59,7 @@ import org.labkey.api.security.SecurityManager.UserManagementException; import org.labkey.api.security.permissions.AbstractActionPermissionTest; import org.labkey.api.security.permissions.ApplicationAdminPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.permissions.SiteAdminPermission; import org.labkey.api.security.roles.ApplicationAdminRole; import org.labkey.api.security.roles.SiteAdminRole; @@ -162,7 +162,7 @@ public static void addUserListener(UserListener listener) public static void addUserListener(UserListener listener, boolean meFirst) { if (meFirst) - _listeners.add(0, listener); + _listeners.addFirst(listener); else _listeners.add(listener); } @@ -670,12 +670,14 @@ public static String getEmailForId(Integer userId) } @NotNull + // Returns a mutable collection public static Collection getActiveUsers() { return getUsers(false); } @NotNull + // Returns a mutable collection public static Collection getUsers(boolean includeInactive) { return includeInactive ? UserCache.getActiveAndInactiveUsers() : UserCache.getActiveUsers() ; @@ -1305,7 +1307,7 @@ public void testPermissionsRetrieval() assertTrue("Expected all SiteAdmins to be in the AppAdmins list", appAdmins.containsAll(siteAdmins)); - List privilegedUsers = SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), Set.of(CanImpersonatePrivilegedSiteRolesPermission.class)); + List privilegedUsers = SecurityManager.getUsersWithPermissions(ContainerManager.getRoot(), Set.of(ImpersonatePrivilegedSiteRolesPermission.class)); assertTrue("Expected all users with privileged role impersonation permissions to have a privileged role", privilegedUsers.stream().allMatch(User::hasPrivilegedRole)); diff --git a/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java b/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java index e3ea1abbf15..f3663d2163f 100644 --- a/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java +++ b/api/src/org/labkey/api/security/impersonation/AbstractImpersonationContext.java @@ -21,7 +21,7 @@ import org.labkey.api.security.LoginUrls; import org.labkey.api.security.PermissionsContext; import org.labkey.api.security.User; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.roles.Role; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; @@ -29,6 +29,104 @@ import java.util.stream.Stream; +/// +/// The ability to impersonate users, groups, and roles is a powerful and useful security feature of LabKey. +/// Administrators use impersonation to review and troubleshoot their permissions configurations. Developers, testers, +/// and automated tests use impersonation widely to develop and validate LabKey features. This comment documents the +/// rules we've developed over the years to ensure impersonation is useful and secure. +/// +/// ### Guiding Principles +/// Before delving into details, here are some guiding principles we've tried to follow while designing the +/// impersonation features: +/// - Those who can impersonate are already trusted, high-level administrators with wide-ranging permissions. +/// - We strive for flexibility in allowing impersonations. Any restrictions should be there for a good reason. +/// - Starting and stopping impersonation is recorded in the audit log. All key actions taken while impersonating are +/// recorded in the audit log with a clear indication of who was impersonating at the time. +/// - Escalation of permissions is prevented in the vast majority of most cases. We intentionally allow limited +/// exceptions to this and accept the audit log record of impersonators' actions as mitigation for this. +/// +/// ### Three Impersonation Options +/// - Impersonating a user effectively turns the admin into that user. However, the impersonating admin is listed in +/// all audit log entries associated with their actions while impersonating. Admins can't impersonate inactive users +/// or themselves. +/// - Impersonating a group means the admin is still themselves (they'll see their own profile, updates will be tagged +/// with their user ID, etc.), but they are stripped of all role assignments except for the roles granted to three +/// groups: Guests, All Site Users, and the impersonated group. This provides the ability to test the effects of +/// a single group's role assignments in isolation. Admins can't impersonate the Guests group; they can just log out +/// to see the site from a guest's perspective. +/// - Impersonating roles means the admin is still themselves, but they are stripped of all role assignments except +/// the role(s) selected during impersonation. Unlike user and group impersonation (which allow a single principal +/// to be impersonated at a time), admins can impersonate multiple roles. They can also impersonate roles and +/// subsequently adjust that impersonation, adding and removing roles from the impersonation session. Note that many +/// roles don't include read permission; the impersonate roles dialog will encourage (though not require) the +/// addition of a read-granting role if the current selection lacks read permission. +/// +/// ### Three Root Roles Can Impersonate +/// - Three roles that are assigned at the root that can impersonate: Site Administrator, Impersonating Troubleshooter, +/// and Application Admin. +/// - Site Administrators and Impersonating Troubleshooters are all-powerful and already have (or can instantly give +/// themselves) any permission in the system. Other than the exceptions mentioned above, they can impersonate any +/// user, group, or role in the system without restriction. +/// - Application Admins can impersonate the same users and groups as the other two root roles. They can impersonate +/// any role except for three especially powerful roles that are designated as "privileged": Site Administrator, +/// Platform Developer, and Impersonating Troubleshooter. Application Admins can't impersonate privileged roles +/// directly, and when they impersonate a user or group, any privileged roles granted to that principal are filtered +/// out during permission checks. As such, an Application Admin can't take on permissions that are exclusive to a +/// Site Administrator or a Platform Developer through impersonation. +/// - These three root roles can impersonate from the root, in which case they have the option to impersonate any +/// user, any site group, or any site role. They can also impersonate from a project or folder, in which case they +/// can impersonate any user, any site group, any project group from that project, or any role that's applicable to +/// the current folder. When they impersonate, they are free to navigate to any container allowed by their new +/// impersonated permissions. +/// - Troubleshooters and other root roles can't impersonate. +/// +/// ### Project Administrators Can Also Impersonate +/// - Project Administrators can impersonate, but with less freedom than the root roles. Project Administrators are +/// granted most permissions in the system, but only in the context of their project, so their impersonation +/// power is limited accordingly. +/// - Unlike the other three impersonating roles, Project Administrators are completely restricted to the project +/// where impersonation starts. While impersonating, they can't navigate to any other project or the root, no +/// matter what their new impersonation permissions grant. +/// - Project Administrators can impersonate any user with read permissions in the project. This isn't a security +/// restriction, since Project Administrators can grant any user read permissions in their project and impersonate +/// them. This limit is a convenience to focus on just the users with access to that project. Some deployments are +/// partitioned with disjoint sets of users in each project; showing all users would be confusing and inconvenient. +/// Also, it's not very useful to impersonate a user who lacks read permissions. +/// - Project Administrators can impersonate any project group in that project or any site group where they are +/// already a member (though see the first "Considerations" point below). +/// - Project Administrators can impersonate any role that's applicable to the current folder. That means they can't +/// impersonate site roles because they can't impersonate from the root. +/// - Like Application Admins, Project Administrators can't impersonate privileged roles; these roles are filtered +/// out when impersonating a user or group that has them. +/// - Folder Administrators have wide-ranging permissions in their folder, but they can't impersonate. +/// +/// ### Miscellaneous Details +/// - A handful of operations are prohibited while impersonating. These include creating API keys, applying an +/// electronic signature, and impersonating (no chaining of impersonations). +/// - The four admins who can impersonate have automatically been granted the vast majority of permissions in the +/// system, but there are some permissions that must be granted explicitly, even to high-level admins. Areas where +/// admins don't automatically receive permissions include PHI reading and adjudication. For these types of +/// permissions, an admin could escalate their permissions while impersonating, receiving permissions they lack +/// normally (although they could easily grant these permissions to themselves). Also, Project Administrators can +/// impersonate an Application Admin and gain at least one new permission within the project (update user profiles); +/// project sandboxing means they can't take any action as an Application Admin in the root or other projects. In +/// both escalation cases, the audit log tags all actions they take with the impersonating admin's user id. +/// - The UI and the API disallow impersonating site roles and project/folder roles at the same time (though see the +/// second "Considerations" point below). +/// +/// ### Considerations +/// - Eliminate the requirement that Project Administrators must be a member of a site group in order to impersonate +/// it? Project Administrators can already impersonate any user (who could be a member of any group) and we already +/// filter out the privileged roles, so this restriction is hard to justify. +/// - Allow impersonation of site roles from the project and eliminate the prohibition on impersonating site roles +/// and project/folder roles at the same time? Admins can already impersonate users and groups that are granted a +/// mix of roles, and we already filter out the privileged roles, so this restriction seems arbitrary and hard to +/// justify. We would eliminate the isApplicable() check on roles, which would also make it easier to impersonate +/// roles with special requirements, such as EHR- or study-related roles. If we took this step, we could likely +/// combine the permissions-checking methods in RoleImpersonationContextFactory (getValidImpersonationRoles() and +/// verifyPermissions()). +/// + public abstract class AbstractImpersonationContext implements PermissionsContext { private final User _adminUser; @@ -88,7 +186,7 @@ public ImpersonationContextFactory getFactory() */ protected Stream getFilteredRoles(Stream roles) { - if (getAdminUser() != null && !getAdminUser().hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class)) + if (getAdminUser() != null && !getAdminUser().hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class)) return roles.filter(role -> !role.isPrivileged()); return roles; diff --git a/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java index 8580ae943dd..ab3c708a955 100644 --- a/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/GroupImpersonationContextFactory.java @@ -31,7 +31,7 @@ import org.labkey.api.security.SecurityManager; 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.ImpersonatePermission; import org.labkey.api.security.roles.Role; import org.labkey.api.util.GUID; import org.labkey.api.view.ActionURL; @@ -126,33 +126,6 @@ public User getAdminUser() return UserManager.getUser(_adminUserId); } - private static boolean canImpersonateGroup(@Nullable Container project, User user, Group group) - { - // Impersonating the "Site: Guests" group leads to confusion and is not useful. Better to just log out. See #20140. - if (group.isGuests()) - return false; - - // Impersonating "Site: Administrators" or any other group assigned a privileged role by a non-site admin is confusing as well. - if (group.hasPrivilegedRole() && !user.hasSiteAdminPermission()) - return false; - - // Site/app admin can impersonate any other group - if (user.hasRootAdminPermission()) - return true; - - // Project admin... - if (null != project && project.hasPermission(user, AdminPermission.class)) - { - // ...can impersonate any project group but must be a member of a site group to impersonate it - if (group.isProjectGroup()) - return group.getContainer().equals(project.getId()); - else - return user.isInGroup(group.getUserId()); - } - - return false; - } - public static void addMenu(NavTree menu) { NavTree groupMenu = new NavTree("Group"); @@ -160,11 +133,12 @@ public static void addMenu(NavTree menu) menu.addChild(groupMenu); } + // Returns the groups this user is allowed to impersonate. Empty for users who aren't allowed to impersonate. public static Collection getValidImpersonationGroups(Container c, User user) { LinkedList validGroups = new LinkedList<>(); - List groups = SecurityManager.getGroups(c.getProject(), true); Container project = c.getProject(); + List groups = SecurityManager.getGroups(project, true); // Site groups are always first, followed by project groups for (Group group : groups) @@ -174,6 +148,31 @@ public static Collection getValidImpersonationGroups(Container c, User us return validGroups; } + private static boolean canImpersonateGroup(@Nullable Container project, User adminUser, Group group) + { + // Impersonating the "Site: Guests" group leads to confusion and is not useful. Better to just log out. See #20140. + if (group.isGuests()) + return false; + + // Site admin, app admin, and impersonating troubleshooter can impersonate any group, even those assigned + // privileged roles. However, in the case where an app admin impersonates such a group, the privileged roles + // are filtered out (see GroupImpersonationContext.getAssignedRoles() below). + if (adminUser.hasRootPermission(ImpersonatePermission.class)) + return true; + + // Project admin... + if (null != project && project.hasPermission(adminUser, ImpersonatePermission.class)) + { + // ...can impersonate any project group but must be a member of a site group to impersonate it + if (group.isProjectGroup()) + return group.getContainer().equals(project.getId()); + else + return adminUser.isInGroup(group.getUserId()); + } + + return false; + } + private static class GroupImpersonationContext extends AbstractImpersonationContext { private final Group _group; @@ -181,13 +180,13 @@ private static class GroupImpersonationContext extends AbstractImpersonationCont @JsonCreator protected GroupImpersonationContext( - @JsonProperty("_project") @Nullable Container project, - @JsonProperty("_adminUser") User adminUser, - @JsonProperty("_group") Group group, - @JsonProperty("_groups") PrincipalArray groups, - @JsonProperty("_returnUrl") ActionURL returnUrl, - @JsonProperty("_factory") ImpersonationContextFactory factory) - + @JsonProperty("_project") @Nullable Container project, + @JsonProperty("_adminUser") User adminUser, + @JsonProperty("_group") Group group, + @JsonProperty("_groups") PrincipalArray groups, + @JsonProperty("_returnUrl") ActionURL returnUrl, + @JsonProperty("_factory") ImpersonationContextFactory factory + ) { super(adminUser, project, returnUrl, factory); _group = group; diff --git a/api/src/org/labkey/api/security/impersonation/ImpersonationTestCase.java b/api/src/org/labkey/api/security/impersonation/ImpersonationTestCase.java new file mode 100644 index 00000000000..6c6f49ea1a3 --- /dev/null +++ b/api/src/org/labkey/api/security/impersonation/ImpersonationTestCase.java @@ -0,0 +1,157 @@ +package org.labkey.api.security.impersonation; + +import org.apache.logging.log4j.Logger; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.data.ContainerManager; +import org.labkey.api.security.Group; +import org.labkey.api.security.MutableSecurityPolicy; +import org.labkey.api.security.SecurityManager; +import org.labkey.api.security.SecurityManager.UserManagementException; +import org.labkey.api.security.SecurityPolicyManager; +import org.labkey.api.security.User; +import org.labkey.api.security.UserManager; +import org.labkey.api.security.ValidEmail; +import org.labkey.api.security.ValidEmail.InvalidEmailException; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.PlatformDeveloperRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.Role; +import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.util.TestContext; +import org.labkey.api.util.logging.LogHelper; + +import java.util.Collection; +import java.util.List; +import java.util.Set; + +/** + * We have many impersonation permissions rules. This attempts to test many of them. + */ +public class ImpersonationTestCase extends Assert +{ + private static final Logger LOG = LogHelper.getLogger(ImpersonationTestCase.class, "Progress of ImpersonationTest"); + + @Test + public void testPermissions() throws Exception + { + Container root = ContainerManager.getRoot(); + User testUser = TestContext.get().getUser(); + + Container projectToDelete = null; + User noPermUser = null; + User readPermUser = null; + Group readPermGroup = null; + Group privilegedGroup = null; + try + { + // Need to test in a new project since all users get read permission in /Shared, which we don't want + Container project = projectToDelete = ContainerManager.createContainer(root, "_testImpersonationPermissions", testUser); + + // Ensure there's at least one user without permissions in the project + noPermUser = SecurityManager.addUser(new ValidEmail("test_no_permissions@test.com"), null).getUser(); + + // Ensure there's a user assigned read permissions in the project + readPermUser = SecurityManager.addUser(new ValidEmail("test_read_permissions@test.com"), null).getUser(); + MutableSecurityPolicy projectPolicy = new MutableSecurityPolicy(project.getPolicy()); + projectPolicy.addRoleAssignment(readPermUser, ReaderRole.class); + SecurityPolicyManager.savePolicy(projectPolicy, testUser); + + // Ensure there's another project group + readPermGroup = SecurityManager.createGroup(root, "Test_Read_Perm", null); + + // Ensure there's at least one site group assigned to a privileged role + privilegedGroup = SecurityManager.createGroup(root, "Test_Privileged", null); + MutableSecurityPolicy policy = new MutableSecurityPolicy(root.getPolicy()); + Role privilegedRole = RoleManager.getRole(PlatformDeveloperRole.class); + policy.addRoleAssignment(privilegedGroup, privilegedRole); + SecurityPolicyManager.savePolicy(policy, testUser); + + // Site-related counts + int siteUserCount = UserManager.getUserIds().size(); // Includes inactive + List siteGroups = SecurityManager.getGroups(null, false); + int siteGroupCount = siteGroups.size() - 1; // Can't impersonate the guests group + int siteRoleCount = RoleManager.getSiteRoles().size(); + + // Project-related counts + int projectUserCount = SecurityManager.getUsersWithPermissions(project, Set.of(ReadPermission.class)).size(); + assertTrue(projectUserCount < siteUserCount); // Should be at least one less because test_no_permissions@test.com doesn't have read + int projectGroupCount = SecurityManager.getGroups(project, false).size(); + int allGroupCount = siteGroupCount + projectGroupCount; + List projectRoles = RoleManager.getAllRoles().stream().filter(role -> role.isAssignable() && role.isApplicable(projectPolicy, project)).toList(); + int projectRoleCount = projectRoles.size(); + + testImpersonator("SiteAdminRole", true, project, siteUserCount, siteGroupCount, siteRoleCount, siteUserCount, allGroupCount, projectRoleCount); + testImpersonator("ImpersonatingTroubleshooterRole", true, project, siteUserCount, siteGroupCount, siteRoleCount, siteUserCount, allGroupCount, projectRoleCount); + // Can't impersonate privileged roles, so SiteAdmin, ImpersonatingTroubleshooter, and PlatformDeveloper should not appear as valid site roles (siteRoleCount - 3) + // Can impersonate all users and groups (same counts as above), but privileged roles will get filtered out by the impersonation contexts + testImpersonator("ApplicationAdminRole", true, project, siteUserCount, siteGroupCount, siteRoleCount - 3, siteUserCount, allGroupCount, projectRoleCount); + + // Can impersonate any project group plus any site group where project admin is a member. As a brand-new + // user, they are a member in Site:Users only and we created one project group, so they can impersonate two + // groups at the project level. + testImpersonator("ProjectAdminRole", false, project, 0, 0, 0, projectUserCount, 2, projectRoleCount); + + // Can't impersonate anything anywhere + testImpersonator("FolderAdminRole", false, project, 0, 0, 0, 0, 0, 0); + testImpersonator("EditorRole", false, project, 0, 0, 0, 0, 0, 0); + testImpersonator("ReaderRole", false, project, 0, 0, 0, 0, 0, 0); + } + finally + { + if (privilegedGroup != null) + SecurityManager.deleteGroup(privilegedGroup, testUser); + if (readPermGroup != null) + SecurityManager.deleteGroup(readPermGroup, testUser); + if (readPermUser != null) + UserManager.deleteUser(readPermUser.getUserId()); + if (noPermUser != null) + UserManager.deleteUser(noPermUser.getUserId()); + if (projectToDelete != null) + ContainerManager.delete(projectToDelete, testUser); + } + } + + private void testImpersonator(String roleName, boolean siteRole, Container project, int expectedSiteUsers, int expectedSiteGroups, int expectedSiteRoles, int expectedProjectUsers, int expectedProjectGroups, int expectedProjectRoles) throws InvalidEmailException, UserManagementException + { + LOG.info("Testing {}", roleName); + Container root = ContainerManager.getRoot(); + User userToDelete = null; + + try + { + // Create user and assign role + String email = roleName + "@test.com"; + User user = userToDelete = SecurityManager.addUser(new ValidEmail(email), null).getUser(); + Role role = RoleManager.getRole("org.labkey.api.security.roles." + roleName); + assertNotNull("Could not resolve " + roleName, role); + Container roleContainer = siteRole ? root : project; + MutableSecurityPolicy rootPolicy = new MutableSecurityPolicy(roleContainer.getPolicy()); + rootPolicy.addRoleAssignment(user, role); + SecurityPolicyManager.savePolicyForTests(rootPolicy, TestContext.get().getUser()); + + // Test impersonating at the site level + Collection validUsers = UserImpersonationContextFactory.getValidImpersonationUsers(null, user); + assertEquals(expectedSiteUsers, validUsers.size()); + assertTrue(user + " is able to impersonate themselves!", validUsers.stream().noneMatch(u -> u.equals(user))); + Collection validSiteGroups = GroupImpersonationContextFactory.getValidImpersonationGroups(root, user); + assertEquals(expectedSiteGroups, validSiteGroups.size()); + Collection validSiteRoles = RoleImpersonationContextFactory.getValidImpersonationRoles(ContainerManager.getRoot(), user).toList(); + assertEquals(expectedSiteRoles, validSiteRoles.size()); + + // Test impersonating at the project level + Collection projectUsers = UserImpersonationContextFactory.getValidImpersonationUsers(project, user); + assertEquals(expectedProjectUsers, projectUsers.size()); + Collection validProjectGroups = GroupImpersonationContextFactory.getValidImpersonationGroups(project, user); + assertEquals(expectedProjectGroups, validProjectGroups.size()); + Collection validProjectRoles = RoleImpersonationContextFactory.getValidImpersonationRoles(project, user).toList(); + assertEquals(expectedProjectRoles, validProjectRoles.size()); + } + finally + { + if (userToDelete != null) + UserManager.deleteUser(userToDelete.getUserId()); + } + } +} diff --git a/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java index a273596b4d4..6d8072cb784 100644 --- a/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/RoleImpersonationContextFactory.java @@ -19,6 +19,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import jakarta.servlet.http.HttpServletRequest; +import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.audit.AuditLogService; import org.labkey.api.data.Container; @@ -31,9 +32,8 @@ import org.labkey.api.security.SecurityPolicyManager; 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.CanImpersonatePrivilegedSiteRolesPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.roles.AbstractRootContainerRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; @@ -183,16 +183,72 @@ private static void addMenu(NavTree menu, String text) menu.addChild(newRoleMenu); } - public static Stream getValidImpersonationRoles(Container c, User user) + // Can this user impersonate in this container? + private static boolean canImpersonate(@Nullable Container c, User user) { - SecurityPolicy policy = SecurityPolicyManager.getPolicy(c); - boolean canImpersonatePrivilegedRoles = user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class); - - // Stream the valid roles - return RoleManager.getAllRoles().stream() - .filter(Role::isAssignable) - .filter(role -> role.isApplicable(policy, c)) - .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles); + return user.hasRootPermission(ImpersonatePermission.class) || (c != null && !c.isRoot() && c.hasPermission(user, ImpersonatePermission.class)); + } + + public static Stream getValidImpersonationRoles(@NotNull Container c, User user) + { + if (canImpersonate(c, user)) + { + SecurityPolicy policy = SecurityPolicyManager.getPolicy(c); + boolean canImpersonatePrivilegedRoles = user.hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class); + + // Stream the valid roles + return RoleManager.getAllRoles().stream() + .filter(Role::isAssignable) + .filter(role -> role.isApplicable(policy, c)) + .filter(role -> !role.isPrivileged() || canImpersonatePrivilegedRoles); + } + else + { + return Stream.empty(); + } + } + + // Throws if user is not authorized to impersonate all roles + private static void verifyPermissions(@Nullable Container project, User adminUser, Set roles, ImpersonationContextFactory factory) + { + if (canImpersonate(project, adminUser)) + { + // null project means a root admin is impersonating. Impersonation could have started in the root, project, or folder... we don't know. + if (null == project) + { + // Ensure we have either site roles or project roles, not both. UI prevents this, but crafty admin could + // attempt it by crafting a post with specific class names + if (roles.stream() + .collect(Collectors.groupingBy(role -> role instanceof AbstractRootContainerRole)) + .size() > 1) + { + throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles and project roles at the same time", factory); + } + + if (!adminUser.hasRootPermission(ImpersonatePrivilegedSiteRolesPermission.class)) + { + // Application Administrator is not allowed to impersonate privileged roles + List privileged = roles.stream() + .filter(Role::isPrivileged) + .map(Role::getDisplayName) + .toList(); + + if (!privileged.isEmpty()) + throw new UnauthorizedImpersonationException("You are not allowed to impersonate " + StringUtilsLabKey.joinWithConjunction(privileged, "or"), factory); + } + } + else + { + // Must not be impersonating any site roles + if (roles.stream().anyMatch(role -> (role instanceof AbstractRootContainerRole))) + throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles", factory); + } + } + else + { + // Admin's permissions must have been revoked since impersonation began + throw new UnauthorizedImpersonationException("You are not allowed to impersonate here", factory); + } } public static class RoleImpersonationContext extends AbstractImpersonationContext @@ -202,68 +258,29 @@ public static class RoleImpersonationContext extends AbstractImpersonationContex @JsonCreator private RoleImpersonationContext( - @JsonProperty("_project") @Nullable Container project, - @JsonProperty("_adminUser") User adminUser, - @JsonProperty("_roles") RoleSet roles, - @JsonProperty("_factory") ImpersonationContextFactory factory, - @JsonProperty("_cacheKey") String cacheKey) + @JsonProperty("_project") @Nullable Container project, + @JsonProperty("_adminUser") User adminUser, + @JsonProperty("_roles") RoleSet roles, + @JsonProperty("_factory") ImpersonationContextFactory factory, + @JsonProperty("_cacheKey") String cacheKey + ) { this(project, adminUser, roles, null, factory, cacheKey); } private RoleImpersonationContext( - @Nullable Container project, - User adminUser, - RoleSet roles, - ActionURL returnUrl, - ImpersonationContextFactory factory, - String cacheKey) + @Nullable Container project, + User adminUser, + RoleSet roles, + ActionURL returnUrl, + ImpersonationContextFactory factory, + String cacheKey + ) { super(adminUser, project, returnUrl, factory); _roles = roles; _cacheKey = cacheKey; - verifyPermissions(project, adminUser, _roles.getRoles()); - } - - // Throws if user is not authorized to impersonate all roles - private void verifyPermissions(@Nullable Container project, User user, Set roles) - { - if (null == project) - { - // Ensure we have either site roles or project roles, not both - var map = roles.stream() - .collect(Collectors.groupingBy(role -> role instanceof AbstractRootContainerRole)); - - // UI prevents this, but crafty admin could attempt it by crafting a post with specific class names - if (map.size() > 1) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles and project roles at the same time", getFactory()); - - // Site Administrator and Impersonating Troubleshooter can impersonate any site role - if (user.hasRootPermission(CanImpersonatePrivilegedSiteRolesPermission.class)) - return; - - if (!user.hasRootPermission(CanImpersonateSiteRolesPermission.class)) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles", getFactory()); - - // Application Administrator can impersonate all site roles except the privileged ones - List privileged = roles.stream() - .filter(Role::isPrivileged) - .map(Role::getDisplayName) - .toList(); - - if (!privileged.isEmpty()) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate " + StringUtilsLabKey.joinWithConjunction(privileged, "or"), getFactory()); - } - else - { - // Must have admin permissions in this project - if (!project.hasPermission(user, AdminPermission.class)) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate a role in this project", getFactory()); - - // Must not be impersonating any site roles - if (roles.stream().anyMatch(role -> (role instanceof AbstractRootContainerRole))) - throw new UnauthorizedImpersonationException("You are not allowed to impersonate site roles", getFactory()); - } + verifyPermissions(project, adminUser, _roles.getRoles(), factory); } @Override @@ -285,7 +302,7 @@ public Stream getAssignedRoles(User user, SecurableResource resource) // impersonate the specified roles. See Issue #50248 to understand the "instanceof Container" check. return resource instanceof Container c ? _roles.stream() - .filter(role -> !(role instanceof AbstractRootContainerRole siteRole) || siteRole.isAvailableEverywhere() || c.isRoot()) : + .filter(role -> !(role instanceof AbstractRootContainerRole siteRole) || siteRole.isApplicableOutsideRoot() || c.isRoot()) : Stream.empty(); } diff --git a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java index 8976ed4d42c..686c0c74f1e 100644 --- a/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java +++ b/api/src/org/labkey/api/security/impersonation/UserImpersonationContextFactory.java @@ -30,7 +30,8 @@ import org.labkey.api.security.SecurityManager; 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.ImpersonatePermission; +import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.Role; import org.labkey.api.util.GUID; import org.labkey.api.util.SessionHelper; @@ -40,6 +41,7 @@ import java.util.ArrayList; import java.util.Collection; +import java.util.Set; import java.util.stream.Stream; /** @@ -143,24 +145,29 @@ public static void addMenu(NavTree menu) menu.addChild(userMenu); } - // Somewhat redundant with verifyPermissions() below, but much more expensive in the single-user case. Keep these two methods in sync. - // project == null means return all site users (if authorized) + // Redundant with verifyPermissions() below, but much more expensive in the single-user case. Keep these two methods + // in sync. project == null means return all site users (if authorized). Includes inactive users so we can show them + // grayed out in the UI, but they can't be impersonated. Note that we allow app admins and project admins to + // impersonate site admins and other users with privileged roles, but those roles are filtered out by + // UserImpersonationContext. Project admins are also restricted to their project. public static Collection getValidImpersonationUsers(@Nullable Container project, User adminUser) { + // Must assign a mutable list in all scenarios to allow subsequent remove() call Collection validUsers; - // Site admin can impersonate any active user... - if (null == project) + // Site admin, app admin, and impersonating troubleshooter can impersonate any user, regardless of user's permissions and where impersonation is initiated + if (adminUser.hasRootPermission(ImpersonatePermission.class)) { - validUsers = adminUser.hasRootAdminPermission() ? UserManager.getUsers(true) : new ArrayList<>(0); // Mutable list to allow subsequent remove() call + validUsers = UserManager.getUsers(true); } - else if (!project.hasPermission(adminUser, AdminPermission.class)) + else if (project != null && project.hasPermission(adminUser, ImpersonatePermission.class)) { - validUsers = new ArrayList<>(0); // Mutable list to allow subsequent remove() call + // The read permission check is not for security; it just seems useless to offer impersonation on a user who lacks read. + validUsers = new ArrayList<>(SecurityManager.getUsersWithPermissions(project, true, Set.of(ReadPermission.class))); } else { - validUsers = SecurityManager.getProjectUsers(project); + validUsers = new ArrayList<>(0); } validUsers.remove(adminUser); @@ -168,13 +175,34 @@ else if (!project.hasPermission(adminUser, AdminPermission.class)) return validUsers; } + // Keep in sync with getValidImpersonationUsers() above + private static void verifyPermissions(@Nullable Container project, User impersonatedUser, User adminUser, ImpersonationContextFactory factory) + { + if (impersonatedUser.equals(adminUser)) + throw new UnauthorizedImpersonationException("Can't impersonate yourself", factory); + + if (!impersonatedUser.isActive()) + throw new UnauthorizedImpersonationException("Can't impersonate an inactive user", factory); + + // Site admin, app admin, and impersonating troubleshooter can impersonate anywhere, regardless of user's permissions + if (adminUser.hasRootPermission(ImpersonatePermission.class)) + return; + + // Project admin... + if (project != null && project.hasPermission(adminUser, ImpersonatePermission.class) && project.hasPermission(impersonatedUser, ReadPermission.class)) + return; + + throw new UnauthorizedImpersonationException("Can't impersonate this user", factory); + } + private static class UserImpersonationContext extends AbstractImpersonationContext { @JsonCreator protected UserImpersonationContext( - @JsonProperty("_project") @Nullable Container project, - @JsonProperty("_adminUser") User adminUser, - @JsonProperty("_factory") ImpersonationContextFactory factory) + @JsonProperty("_project") @Nullable Container project, + @JsonProperty("_adminUser") User adminUser, + @JsonProperty("_factory") ImpersonationContextFactory factory + ) { super(adminUser, project, null, factory); } @@ -182,24 +210,7 @@ protected UserImpersonationContext( private UserImpersonationContext(@Nullable Container project, User adminUser, User impersonatedUser, ActionURL returnUrl, ImpersonationContextFactory factory) { super(adminUser, project, returnUrl, factory); - verifyPermissions(project, impersonatedUser, adminUser); - } - - // Keep in sync with getValidImpersonationUsers() above - private void verifyPermissions(@Nullable Container project, User impersonatedUser, User adminUser) - { - if (impersonatedUser.equals(adminUser)) - throw new UnauthorizedImpersonationException("Can't impersonate yourself", getFactory()); - - // Site/app admin can impersonate anywhere - if (adminUser.hasRootAdminPermission()) - return; - - // Project admin... - if (null != project && project.hasPermission(adminUser, AdminPermission.class) && SecurityManager.getProjectUsersIds(project).contains(impersonatedUser.getUserId())) - return; - - throw new UnauthorizedImpersonationException("Can't impersonate this user", getFactory()); + verifyPermissions(project, impersonatedUser, adminUser, factory); } @Override diff --git a/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java b/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java index 40efb642e24..42d1441e186 100644 --- a/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java +++ b/api/src/org/labkey/api/security/permissions/AbstractActionPermissionTest.java @@ -67,6 +67,7 @@ public abstract class AbstractActionPermissionTest extends Assert private static final String AUTHOR_EMAIL = "author@actionpermission.test"; private static final String READER_EMAIL = "reader@actionpermission.test"; private static final String SUBMITTER_EMAIL = "submitter@actionpermission.test"; + private static final String NO_PERMISSION_EMAIL = "nopermission@actionpermission.test"; private static final String TRUSTED_EDITOR_EMAIL = "trustededitor@actionpermission.test"; private static final String TRUSTED_AUTHOR_EMAIL = "trustedauthor@actionpermission.test"; private static final String TROUBLESHOOTER_EMAIL = "troubleshooter@actionpermission.test"; @@ -74,7 +75,7 @@ public abstract class AbstractActionPermissionTest extends Assert protected static final String[] LKS_ROLE_EMAILS = { SITE_ADMIN_EMAIL, APPLICATION_ADMIN_EMAIL, PROJECT_ADMIN_EMAIL, FOLDER_ADMIN_EMAIL, EDITOR_EMAIL, AUTHOR_EMAIL, READER_EMAIL, SUBMITTER_EMAIL, TRUSTED_EDITOR_EMAIL, TRUSTED_AUTHOR_EMAIL, TROUBLESHOOTER_EMAIL, - IMPERSONATING_TROUBLESHOOTER_EMAIL + IMPERSONATING_TROUBLESHOOTER_EMAIL, NO_PERMISSION_EMAIL }; protected static Container _c; @@ -183,6 +184,21 @@ public static Map createUsers(String[] userEmails) return users; } + public void assertForNoPermission(User user, PermissionCheckableAction... actions) + { + for (PermissionCheckableAction action : actions) + { + assertPermission(_c, action, user); + + assertPermission(_c, action, + _users.get(READER_EMAIL), _users.get(AUTHOR_EMAIL), _users.get(EDITOR_EMAIL), + _users.get(FOLDER_ADMIN_EMAIL), _users.get(PROJECT_ADMIN_EMAIL), + _users.get(APPLICATION_ADMIN_EMAIL), _users.get(SITE_ADMIN_EMAIL), + _users.get(SUBMITTER_EMAIL) + ); + } + } + public void assertForReadPermission(User user, PermissionCheckableAction... actions) { assertForReadPermission(user, false, actions); diff --git a/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java b/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java index c5ca174f05a..f7bb5f57421 100644 --- a/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java +++ b/api/src/org/labkey/api/security/permissions/AdminOperationsPermission.java @@ -16,7 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to manage operational site administration settings, such as arbitrary paths on the server's + * Provides the ability to manage operational site administration settings, such as arbitrary paths on the server's * underlying file system. Use {@see org.labkey.api.security.permissions.ApplicationAdminPermission} for gating * access to server-wide settings that don't have the potential to access these underlying server resources. */ diff --git a/api/src/org/labkey/api/security/permissions/AdminPermission.java b/api/src/org/labkey/api/security/permissions/AdminPermission.java index eec7016aa03..e19c2ad60c8 100644 --- a/api/src/org/labkey/api/security/permissions/AdminPermission.java +++ b/api/src/org/labkey/api/security/permissions/AdminPermission.java @@ -25,11 +25,8 @@ import java.util.Set; /** - * Describes the ability to perform administration. Note that this is distinct from {@link org.labkey.api.security.roles.SiteAdminRole}, + * Provides the ability to perform administration. Note that this is distinct from {@link org.labkey.api.security.roles.SiteAdminRole}, * which is far more all-encompassing. - * - * User: Dave - * Date: Apr 28, 2009 */ public class AdminPermission extends AbstractPermission { diff --git a/api/src/org/labkey/api/security/permissions/CanImpersonateSiteRolesPermission.java b/api/src/org/labkey/api/security/permissions/CanImpersonateSiteRolesPermission.java deleted file mode 100644 index 47bbf29dca7..00000000000 --- a/api/src/org/labkey/api/security/permissions/CanImpersonateSiteRolesPermission.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.labkey.api.security.permissions; - -public class CanImpersonateSiteRolesPermission extends AbstractPermission -{ - public CanImpersonateSiteRolesPermission() - { - super("Can Impersonate Site Roles", "Allows users to impersonate site roles except for privileged roles"); - } -} diff --git a/api/src/org/labkey/api/security/permissions/DeletePermission.java b/api/src/org/labkey/api/security/permissions/DeletePermission.java index d36a7ff08e3..855338d1a06 100644 --- a/api/src/org/labkey/api/security/permissions/DeletePermission.java +++ b/api/src/org/labkey/api/security/permissions/DeletePermission.java @@ -16,9 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to delete some sort of object. - * User: Dave - * Date: Apr 27, 2009 + * Provides the ability to delete objects */ public class DeletePermission extends AbstractPermission { diff --git a/api/src/org/labkey/api/security/permissions/ImpersonatePermission.java b/api/src/org/labkey/api/security/permissions/ImpersonatePermission.java new file mode 100644 index 00000000000..26ee98a3351 --- /dev/null +++ b/api/src/org/labkey/api/security/permissions/ImpersonatePermission.java @@ -0,0 +1,9 @@ +package org.labkey.api.security.permissions; + +public class ImpersonatePermission extends AbstractPermission +{ + public ImpersonatePermission() + { + super("Can Impersonate", "Allows users to impersonate users, groups, and roles, except for privileged roles"); + } +} diff --git a/api/src/org/labkey/api/security/permissions/CanImpersonatePrivilegedSiteRolesPermission.java b/api/src/org/labkey/api/security/permissions/ImpersonatePrivilegedSiteRolesPermission.java similarity index 57% rename from api/src/org/labkey/api/security/permissions/CanImpersonatePrivilegedSiteRolesPermission.java rename to api/src/org/labkey/api/security/permissions/ImpersonatePrivilegedSiteRolesPermission.java index 4c4454a3256..61e31d0fe35 100644 --- a/api/src/org/labkey/api/security/permissions/CanImpersonatePrivilegedSiteRolesPermission.java +++ b/api/src/org/labkey/api/security/permissions/ImpersonatePrivilegedSiteRolesPermission.java @@ -1,8 +1,8 @@ package org.labkey.api.security.permissions; -public class CanImpersonatePrivilegedSiteRolesPermission extends AbstractPermission +public class ImpersonatePrivilegedSiteRolesPermission extends AbstractPermission { - public CanImpersonatePrivilegedSiteRolesPermission() + public ImpersonatePrivilegedSiteRolesPermission() { super("Can Impersonate Privileged Site Roles", "Allows users to impersonate privileged site roles including Site Admin"); } diff --git a/api/src/org/labkey/api/security/permissions/InsertPermission.java b/api/src/org/labkey/api/security/permissions/InsertPermission.java index 74bed03132a..93e05a548b7 100644 --- a/api/src/org/labkey/api/security/permissions/InsertPermission.java +++ b/api/src/org/labkey/api/security/permissions/InsertPermission.java @@ -18,9 +18,7 @@ import org.jetbrains.annotations.NotNull; /** - * Describes the ability to add new objects to the system. - * User: Dave - * Date: Apr 27, 2009 + * Provides the ability to add new objects to the system */ public class InsertPermission extends AbstractPermission { diff --git a/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java b/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java index 3e36d9e7122..6571df30090 100644 --- a/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java +++ b/api/src/org/labkey/api/security/permissions/MoveEntitiesPermission.java @@ -16,7 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to move entities between containers within the system. + * Provides the ability to move entities between containers within the system */ public class MoveEntitiesPermission extends UpdatePermission { diff --git a/api/src/org/labkey/api/security/permissions/ReadPermission.java b/api/src/org/labkey/api/security/permissions/ReadPermission.java index 4a9efc8e3cf..4664db9e7c8 100644 --- a/api/src/org/labkey/api/security/permissions/ReadPermission.java +++ b/api/src/org/labkey/api/security/permissions/ReadPermission.java @@ -18,9 +18,7 @@ import org.jetbrains.annotations.NotNull; /** - * Describes the ability to view information within the system. - * User: Dave - * Date: Apr 23, 2009 + * Provides the ability to view information within the system */ @AllowedForReadOnlyUser public class ReadPermission extends AbstractPermission diff --git a/api/src/org/labkey/api/security/permissions/ReadSomePermission.java b/api/src/org/labkey/api/security/permissions/ReadSomePermission.java index 686ea9b95e9..ba9cab4db27 100644 --- a/api/src/org/labkey/api/security/permissions/ReadSomePermission.java +++ b/api/src/org/labkey/api/security/permissions/ReadSomePermission.java @@ -16,9 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to read a limited subset of information within a specific context. - * User: Dave - * Date: Apr 30, 2009 + * Provides the ability to read a limited subset of information within a specific context */ @AllowedForReadOnlyUser public class ReadSomePermission extends AbstractPermission diff --git a/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java b/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java index b1d4be92641..4c54506e959 100644 --- a/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java +++ b/api/src/org/labkey/api/security/permissions/TroubleshooterPermission.java @@ -17,7 +17,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to view administration and configuration, but not change it. + * Provides the ability to view administration and configuration, but not change it */ @AllowedForReadOnlyUser public class TroubleshooterPermission extends AbstractPermission diff --git a/api/src/org/labkey/api/security/permissions/UpdatePermission.java b/api/src/org/labkey/api/security/permissions/UpdatePermission.java index 034d0a65a10..2179b29faaa 100644 --- a/api/src/org/labkey/api/security/permissions/UpdatePermission.java +++ b/api/src/org/labkey/api/security/permissions/UpdatePermission.java @@ -18,9 +18,7 @@ import org.jetbrains.annotations.NotNull; /** - * Describes the ability to change existing objects within the system. - * User: Dave - * Date: Apr 27, 2009 + * Provides the ability to change existing objects within the system */ public class UpdatePermission extends AbstractPermission { diff --git a/api/src/org/labkey/api/security/permissions/UserManagementPermission.java b/api/src/org/labkey/api/security/permissions/UserManagementPermission.java index c5d74da6442..47bce2b2db4 100644 --- a/api/src/org/labkey/api/security/permissions/UserManagementPermission.java +++ b/api/src/org/labkey/api/security/permissions/UserManagementPermission.java @@ -16,7 +16,7 @@ package org.labkey.api.security.permissions; /** - * Describes the ability to manage users (create, delete, deactivate) for the server. + * Provides the ability to manage users (create, delete, deactivate) */ public class UserManagementPermission extends AdminPermission { diff --git a/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java b/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java index 4ccb841b53a..b5816bd6a7b 100644 --- a/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java +++ b/api/src/org/labkey/api/security/roles/AbstractRootContainerRole.java @@ -61,7 +61,9 @@ public boolean isApplicable(SecurityPolicy policy, SecurableResource resource) return resource instanceof Container && ((Container)resource).isRoot(); } - public boolean isAvailableEverywhere() + // Most site roles are applicable outside the root (i.e., every container). Troubleshooters are an exception + // because we want them to have read in the root but not elsewhere. + public boolean isApplicableOutsideRoot() { return true; } diff --git a/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java b/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java index 8c15081827b..6923b44f1d7 100644 --- a/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java +++ b/api/src/org/labkey/api/security/roles/ApplicationAdminRole.java @@ -17,10 +17,10 @@ import org.labkey.api.security.permissions.AddUserPermission; import org.labkey.api.security.permissions.ApplicationAdminPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; import org.labkey.api.security.permissions.DeleteUserPermission; import org.labkey.api.security.permissions.EnableRestrictedModules; import org.labkey.api.security.permissions.ExemptFromAccountDisablingPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.TroubleshooterPermission; import org.labkey.api.security.permissions.UpdateUserPermission; @@ -37,10 +37,10 @@ public class ApplicationAdminRole extends AbstractRootContainerRole implements A static Collection> PERMISSIONS = Arrays.asList( AddUserPermission.class, ApplicationAdminPermission.class, - CanImpersonateSiteRolesPermission.class, DeleteUserPermission.class, EnableRestrictedModules.class, ExemptFromAccountDisablingPermission.class, + ImpersonatePermission.class, TroubleshooterPermission.class, UpdateUserPermission.class, UserManagementPermission.class diff --git a/api/src/org/labkey/api/security/roles/CanSeeAuditLogFolderRole.java b/api/src/org/labkey/api/security/roles/CanSeeAuditLogFolderRole.java new file mode 100644 index 00000000000..9ebd470280c --- /dev/null +++ b/api/src/org/labkey/api/security/roles/CanSeeAuditLogFolderRole.java @@ -0,0 +1,16 @@ +package org.labkey.api.security.roles; + +import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; + +/** + * See {@link CanSeeAuditLogRole} for the site role version + */ +public class CanSeeAuditLogFolderRole extends AbstractRole +{ + protected CanSeeAuditLogFolderRole() + { + super("See Audit Log Events", "Allows non-administrators to view audit log events. " + CanSeeAuditLogRole.FINAL_WARNING_LINE, + CanSeeAuditLogPermission.class + ); + } +} diff --git a/api/src/org/labkey/api/security/roles/CanSeeAuditLogRole.java b/api/src/org/labkey/api/security/roles/CanSeeAuditLogRole.java index 3a0bddf8fb6..f033329e500 100644 --- a/api/src/org/labkey/api/security/roles/CanSeeAuditLogRole.java +++ b/api/src/org/labkey/api/security/roles/CanSeeAuditLogRole.java @@ -18,11 +18,20 @@ import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.security.permissions.SeeUserDetailsPermission; +/** + * See {@link CanSeeAuditLogFolderRole}, the project/folder version of this role + */ public class CanSeeAuditLogRole extends AbstractRootContainerRole { + static final String FINAL_WARNING_LINE = "This role should be used with caution since the audit log may " + + "contain sensitive or protected information. For example, dataset or list imports where detailed logging " + + "was turned on."; + public CanSeeAuditLogRole() { - super("See Audit Log Events", "Allows non-administrators to view audit log events", + super("See Audit Log Events", "Allows non-administrators to view audit log events in the " + + "root, every project, and every folder on this site. This level of visibility is not generally recommended. " + + "For more granular control, assign this role at the folder level instead. " + FINAL_WARNING_LINE, CanSeeAuditLogPermission.class, SeeUserDetailsPermission.class ); diff --git a/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java b/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java index f6bdfcad095..fc04c2d0c8b 100644 --- a/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java +++ b/api/src/org/labkey/api/security/roles/ImpersonatingTroubleshooterRole.java @@ -1,7 +1,7 @@ package org.labkey.api.security.roles; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.ExemptFromAccountDisablingPermission; import java.util.Set; @@ -10,12 +10,12 @@ public class ImpersonatingTroubleshooterRole extends AbstractRootContainerRole { protected ImpersonatingTroubleshooterRole() { - super("Impersonating Troubleshooter", "Can impersonate site roles, including Site Administrator, in addition to having other standard Troubleshooter abilities.", + super("Impersonating Troubleshooter", "Can impersonate users, groups, and roles, including Site Administrator, in addition to having other standard Troubleshooter abilities.", TroubleshooterRole.PERMISSIONS, Set.of( - CanImpersonatePrivilegedSiteRolesPermission.class, - CanImpersonateSiteRolesPermission.class, - ExemptFromAccountDisablingPermission.class + ExemptFromAccountDisablingPermission.class, + ImpersonatePrivilegedSiteRolesPermission.class, + ImpersonatePermission.class ) ); excludeUsers(); @@ -28,7 +28,7 @@ public boolean isPrivileged() } @Override - public boolean isAvailableEverywhere() + public boolean isApplicableOutsideRoot() { return false; } diff --git a/api/src/org/labkey/api/security/roles/ProjectAdminRole.java b/api/src/org/labkey/api/security/roles/ProjectAdminRole.java index b225d5683fb..2ee1d953a4f 100644 --- a/api/src/org/labkey/api/security/roles/ProjectAdminRole.java +++ b/api/src/org/labkey/api/security/roles/ProjectAdminRole.java @@ -19,9 +19,10 @@ import org.labkey.api.security.SecurableResource; import org.labkey.api.security.SecurityPolicy; import org.labkey.api.security.permissions.AddUserPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.Permission; -import java.util.Collections; +import java.util.List; public class ProjectAdminRole extends AbstractRole implements AdminRoleListener { @@ -30,7 +31,10 @@ public ProjectAdminRole() super("Project Administrator", "Project Administrators have full control over the project, but not the entire system.", FolderAdminRole.PERMISSIONS, - Collections.singletonList(AddUserPermission.class) + List.of( + AddUserPermission.class, + ImpersonatePermission.class + ) ); excludeGuests(); diff --git a/api/src/org/labkey/api/security/roles/Role.java b/api/src/org/labkey/api/security/roles/Role.java index 64e56dbc2e9..c5a1666c77c 100644 --- a/api/src/org/labkey/api/security/roles/Role.java +++ b/api/src/org/labkey/api/security/roles/Role.java @@ -121,8 +121,8 @@ default boolean isExcludedPrincipal(@NotNull UserPrincipal principal) } /** - * @return Whether this role is applicable to the policy. For example, some roles might only make sense in the context of a - * certain type of resource, such as a folder (or particular type of folder) or dataset + * @return Whether this role is applicable in this resource. For example, some roles might only make sense in the + * context of a dataset or a certain folder type. TODO: Eliminate policy parameter; no implementation uses it. */ boolean isApplicable(SecurityPolicy policy, SecurableResource resource); diff --git a/api/src/org/labkey/api/security/roles/RoleManager.java b/api/src/org/labkey/api/security/roles/RoleManager.java index d656e5592d9..8ba6de628b1 100644 --- a/api/src/org/labkey/api/security/roles/RoleManager.java +++ b/api/src/org/labkey/api/security/roles/RoleManager.java @@ -134,6 +134,7 @@ private int getPermLevel(Role r) registerRole(new SubmitterRole()); registerRole(new NoPermissionsRole()); registerRole(new OwnerRole()); + registerRole(new CanSeeAuditLogFolderRole()); } public static void addAdminRoleListener(AdminRoleListener listener) diff --git a/api/src/org/labkey/api/security/roles/SiteAdminRole.java b/api/src/org/labkey/api/security/roles/SiteAdminRole.java index df248dd1bec..063452eb844 100644 --- a/api/src/org/labkey/api/security/roles/SiteAdminRole.java +++ b/api/src/org/labkey/api/security/roles/SiteAdminRole.java @@ -16,11 +16,9 @@ package org.labkey.api.security.roles; import org.labkey.api.security.permissions.AdminOperationsPermission; -import org.labkey.api.security.permissions.CanImpersonatePrivilegedSiteRolesPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; +import org.labkey.api.security.permissions.ImpersonatePrivilegedSiteRolesPermission; import org.labkey.api.security.permissions.CanUseSendMessageApiPermission; import org.labkey.api.security.permissions.EditModuleResourcesPermission; -import org.labkey.api.security.permissions.ExemptFromAccountDisablingPermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.SiteAdminPermission; import org.labkey.api.security.permissions.UploadFileBasedModulePermission; @@ -37,11 +35,9 @@ public class SiteAdminRole extends AbstractRootContainerRole implements AdminRol { private static final Collection> PERMISSIONS = Arrays.asList( AdminOperationsPermission.class, - CanImpersonatePrivilegedSiteRolesPermission.class, - CanImpersonateSiteRolesPermission.class, CanUseSendMessageApiPermission.class, EmailNonUsersPermission.class, - ExemptFromAccountDisablingPermission.class, + ImpersonatePrivilegedSiteRolesPermission.class, SiteAdminPermission.class, UploadFileBasedModulePermission.class ); diff --git a/api/src/org/labkey/api/security/roles/TroubleshooterRole.java b/api/src/org/labkey/api/security/roles/TroubleshooterRole.java index 03c4da04936..b59d58867c9 100644 --- a/api/src/org/labkey/api/security/roles/TroubleshooterRole.java +++ b/api/src/org/labkey/api/security/roles/TroubleshooterRole.java @@ -39,7 +39,7 @@ public TroubleshooterRole() } @Override - public boolean isAvailableEverywhere() + public boolean isApplicableOutsideRoot() { return false; // This ensures troubleshooters get these permissions (esp. ReadPermission) only in the root } diff --git a/api/src/org/labkey/api/util/URLHelper.java b/api/src/org/labkey/api/util/URLHelper.java index 2eeadcfb3b5..65a18fc3d42 100644 --- a/api/src/org/labkey/api/util/URLHelper.java +++ b/api/src/org/labkey/api/util/URLHelper.java @@ -915,7 +915,7 @@ private static boolean isAllowedExternalHost(@NotNull String host, boolean devMo if (devMode && "localhost".equalsIgnoreCase(host)) return true; - // Count the dots in case there's a wild card + // Count the dots in case there's a wildcard int dotCount = StringUtils.countMatches(host, '.'); return allowedHostsSupplier.get().stream() @@ -928,14 +928,14 @@ private static boolean isMatch(@NotNull String host, int dotCount, @NotNull Stri if (allowedHost.startsWith("*.")) { - // This wild-card pattern matches the host if 1) they have the same number of dots and 2) the host ends with - // the portion of the pattern after the wild card. + // This wildcard pattern matches the host if 1) they have the same number of dots and 2) the host ends with + // the portion of the pattern after the wildcard and dot. int expectedDotCount = StringUtils.countMatches(allowedHost, '.'); ret = (dotCount == expectedDotCount && Strings.CI.endsWith(host, allowedHost.substring(2))); } else { - // Non-wild-card pattern must match the entire host + // Non-wildcard pattern must match the entire host ret = Strings.CI.equals(host, allowedHost); } @@ -1120,8 +1120,8 @@ public void testAllowedHosts() allowedHosts2.add("www.google.com"); assertTrue(isAllowedExternalHost("www.google.com", true, ()->allowedHosts2)); - // test wild cards - List wildCardHosts = List.of( + // test wildcards + List wildcardHosts = List.of( "*.labkey.com", "labkey.com", "*.lkpoc.labkey.com", @@ -1130,21 +1130,21 @@ public void testAllowedHosts() ); // good - assertTrue(isAllowedExternalHost("www.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("lkpoc.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub1.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub2.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub1.lkpoc.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub2.lkpoc.labkey.com", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub1.trial.labkey.host", true, ()->wildCardHosts)); - assertTrue(isAllowedExternalHost("sub2.trial.labkey.host", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("www.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("lkpoc.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub1.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub2.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub1.lkpoc.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub2.lkpoc.labkey.com", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub1.trial.labkey.host", true, ()->wildcardHosts)); + assertTrue(isAllowedExternalHost("sub2.trial.labkey.host", true, ()->wildcardHosts)); // bad - assertFalse(isAllowedExternalHost("trial.labkey.host", true, ()->wildCardHosts)); - assertFalse(isAllowedExternalHost("sub1.sub2.lkpoc.labkey.com", true, ()->wildCardHosts)); - assertFalse(isAllowedExternalHost("google.com", true, ()->wildCardHosts)); - assertFalse(isAllowedExternalHost("sub1.sub2.labkey.com", true, ()->wildCardHosts)); + assertFalse(isAllowedExternalHost("trial.labkey.host", true, ()->wildcardHosts)); + assertFalse(isAllowedExternalHost("sub1.sub2.lkpoc.labkey.com", true, ()->wildcardHosts)); + assertFalse(isAllowedExternalHost("google.com", true, ()->wildcardHosts)); + assertFalse(isAllowedExternalHost("sub1.sub2.labkey.com", true, ()->wildcardHosts)); } } } diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index c28e8ebe84c..90a9b1c5437 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -10,6 +10,7 @@ import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.collections4.SetValuedMap; import org.apache.commons.collections4.multimap.HashSetValuedHashMap; +import org.apache.commons.lang3.EnumUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Strings; import org.apache.logging.log4j.Logger; @@ -19,7 +20,6 @@ import org.junit.Test; import org.labkey.api.admin.AdminUrls; import org.labkey.api.collections.CopyOnWriteHashMap; -import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.security.Directive; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.OptionalFeatureService; @@ -42,6 +42,8 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -95,6 +97,11 @@ public String getHeaderName() { return _headerName; } + + private static @Nullable ContentSecurityPolicyType get(String disposition) + { + return EnumUtils.getEnumIgnoreCase(ContentSecurityPolicyType.class, disposition); + } } static @@ -119,8 +126,9 @@ public void init(FilterConfig filterConfig) throws ServletException String paramValue = filterConfig.getInitParameter(paramName); if ("policy".equalsIgnoreCase(paramName)) { + // Extract before filtering since CSP version is in a comment + extractCspVersion(paramValue); _stashedTemplate = filterPolicy(paramValue); - extractCspVersion(_stashedTemplate); } else if ("disposition".equalsIgnoreCase(paramName)) { @@ -139,12 +147,12 @@ else if ("disposition".equalsIgnoreCase(paramName)) if (CSP_FILTERS.put(getType(), this) != null) throw new ServletException("ContentSecurityPolicyFilter is misconfigured, duplicate policies of type: " + getType()); - // configure a different endpoint for each type to convey the correct csp version (eXX vs. rXX) + // configure a different endpoint for each type. TODO: We only need one CSP violation reporting endpoint now, so one header would do _reportToEndpointName = "csp-" + getType().name().toLowerCase(); } /** Filter out block comments and replace special characters in the provided policy */ - public static String filterPolicy(String policy) + private static String filterPolicy(String policy) { String s = policy.trim(); s = s.replace( '\n', ' ' ); @@ -164,40 +172,24 @@ public static String filterPolicy(String policy) return s; } + private static final Pattern CSP_VERSION_PATTERN = Pattern.compile("cspVersion\\s*=\\s*(\\w+)"); + /** - * Extract the cspVersion parameter value from the report-uri directive, if possible. Otherwise, cspVersion is left - * as "Unknown". This value is reported as part of usage metrics. + * Extract the cspVersion value from a comment in the CSP, if it exists. Otherwise, cspVersion is left as "Unknown". + * This value is reported as part of usage metrics and included in violation reports that are logged and forwarded. */ private void extractCspVersion(String s) { - // Simple parser that should be compliant with https://www.w3.org/TR/CSP3/#parse-serialized-policy - Map cspMap = Arrays.stream(s.split(";")) - .map(String::trim) - .filter(line -> !line.isEmpty()) - .map(line -> line.split("\\s+", 2)) - .filter(parts -> parts.length == 2) - .collect(LabKeyCollectors.toCaseInsensitiveLinkedMap(parts -> parts[0], parts -> parts[1])); - - String directive = "report-uri"; - String reportUri = cspMap.get(directive); - - if (reportUri != null) + Matcher matcher = CSP_VERSION_PATTERN.matcher(s); + if (matcher.find()) { - try - { - ActionURL reportUrl = new ActionURL(reportUri); - String cspVersion = reportUrl.getParameter("cspVersion"); + _cspVersion = matcher.group(1); - if (null != cspVersion) - _cspVersion = cspVersion; - } - catch (IllegalArgumentException e) - { - LOG.warn("Unable to parse {} URI", directive, e); - } - } + if (matcher.find()) + LOG.warn("More than one cspVersion=XX assignment found; using the first one."); - LOG.debug("CspVersion: {}", getCspVersion()); + LOG.debug("CspVersion: {}", getCspVersion()); + } } @Override @@ -277,7 +269,7 @@ private CspFilterSettings(ContentSecurityPolicyFilter filter, String baseServerU { // Each filter adds its own "Reporting-Endpoints" header since we want to convey the correct version (eXX vs. rXX) @SuppressWarnings("DataFlowIssue") - ActionURL violationUrl = PageFlowUtil.urlProvider(AdminUrls.class).getCspReportToURL(filter.getCspVersion()); + ActionURL violationUrl = PageFlowUtil.urlProvider(AdminUrls.class).getCspReportToURL(); // Use an absolute URL so we always post to https:, even if the violating request uses http: _reportingEndpointsHeaderValue = filter.getReportToEndpointName() + "=\"" + violationUrl.getURIString() + "\""; @@ -406,6 +398,34 @@ public static boolean hasCsp(ContentSecurityPolicyType type) return CSP_FILTERS.get(type) != null; } + public static @NotNull String getCspVersion(@Nullable String disposition) + { + if (disposition != null) + { + ContentSecurityPolicyType type = ContentSecurityPolicyType.get(disposition); + + if (type != null) + { + var filter = CSP_FILTERS.get(type); + + if (null != filter) + { + return filter.getCspVersion(); + } + else + { + LOG.error("Disposition {} doesn't match a configured CSP filter", disposition); + } + } + else + { + LOG.error("Bad disposition: {}", disposition); + } + } + + return "Unknown"; + } + public static List getMissingSubstitutions(ContentSecurityPolicyType type) { ContentSecurityPolicyFilter filter = CSP_FILTERS.get(type); @@ -442,6 +462,32 @@ public static void registerMetricsProvider() public static class TestCase extends Assert { + @Test + public void testCspVersionExtraction() + { + testCspExtract("e14", "/* cspVersion=e14 */"); + testCspExtract("r14", "/*cspVersion=r14 */"); + testCspExtract("e15", "/* cspVersion = e15 */"); + testCspExtract("r15", "/* cspVersion=r15*/"); + testCspExtract("e15", "/* cspVersion = e15*/"); + testCspExtract("e15", "/* cspVersion = e15*/ /* cspVersion=XXX */"); + + testCspExtract("Unknown", ""); + testCspExtract("Unknown", " "); + testCspExtract("Unknown", "/* cspVersin=e14 */"); + testCspExtract("Unknown", "/* cspVersion */"); + testCspExtract("Unknown", "/* cspVersion= */"); + testCspExtract("Unknown", "/* cspVersion=*/"); + testCspExtract("Unknown", "/* cspVersion== */"); + } + + private void testCspExtract(String expected, String csp) + { + ContentSecurityPolicyFilter filter = new ContentSecurityPolicyFilter(); + filter.extractCspVersion(csp); + assertEquals(expected, filter.getCspVersion()); + } + @Test public void testPolicyFiltering() { @@ -461,7 +507,7 @@ public void testPolicyFiltering() report-uri /* Whoa! */ /admin-contentsecuritypolicyreport.api?${CSP.REPORT.PARAMS} https://*; """; - // Multi-line for readability, but notice that newlines are replaced before assignment + // Multi-line for readability, but notice that newlines are replaced when constructing the expected string String expected = """ default-src 'self' https: http: ; connect-src 'self' http://www.labkey.org localhost:* ws: ${LABKEY.ALLOWED.CONNECTIONS} ; diff --git a/assay/src/org/labkey/assay/AssayManager.java b/assay/src/org/labkey/assay/AssayManager.java index 1fa842571d8..4d0302e937d 100644 --- a/assay/src/org/labkey/assay/AssayManager.java +++ b/assay/src/org/labkey/assay/AssayManager.java @@ -378,10 +378,16 @@ public AssaySchema createSchema(User user, Container container, @Nullable Contai @Override public @NotNull List getAssayProtocols(Container container) + { + return getAssayProtocols(container, false); + } + + private @NotNull List getAssayProtocols(Container container, boolean currentOnly) { List allProtocols = new ArrayList<>(); - for (Container containerInScope : container.getContainersFor(ContainerType.DataType.protocol)) + Collection containerScopes = currentOnly ? List.of(container) : container.getContainersFor(ContainerType.DataType.protocol); + for (Container containerInScope : containerScopes) { List ids = PROTOCOL_CACHE.get(containerInScope); allProtocols.addAll(ids); @@ -622,14 +628,14 @@ public ExpExperiment findBatch(ExpRun run) } /** - * Creates a single document per assay design/folder combo, with some simple assay info (name, description), plus - * the names and comments from all the runs. + * Creates a single document per assay design, with some simple assay info (name, description). + * Note: the document for an assay protocol will just be associated with the protocol's container */ @Override public void indexAssays(SearchService.TaskIndexingQueue queue) { - List protocols = getAssayProtocols(queue.getContainer()); - + // GitHub Issue 895: for the assay protocol search document just user the current container's protocols + List protocols = getAssayProtocols(queue.getContainer(), true); for (ExpProtocol protocol : protocols) indexAssay(queue, protocol); } @@ -661,22 +667,6 @@ public void indexAssay(SearchService.TaskIndexingQueue queue, ExpProtocol protoc m.put(SearchService.PROPERTY.keywordsMed.toString(), keywords); m.put(SearchService.PROPERTY.categories.toString(), ASSAY_CATEGORY.getName()); - ExperimentService.get().getExpRuns(c, protocol, null) - .forEach(run -> { - StringBuilder runKeywords = new StringBuilder(); - - runKeywords.append(" "); - runKeywords.append(run.getName()); - - if (null != run.getComments()) - { - runKeywords.append(" "); - runKeywords.append(run.getComments()); - } - - body.append(runKeywords); - }); - String docId = protocol.getDocumentId(); WebdavResource r = new SimpleDocumentResource(new Path(docId), docId, c.getEntityId(), "text/plain", body.toString(), assayBeginURL, createdBy, created, modifiedBy, modified, m); queue.addResource(r); @@ -728,7 +718,7 @@ private void indexAssayBatches(SearchService.TaskIndexingQueue queue, ExpProtoco { if (shouldIndexProtocolBatches(protocol)) { - for (ExpExperiment batch : protocol.getBatches()) + for (ExpExperiment batch : protocol.getBatches(queue.getContainer())) { if (modifiedSince == null || modifiedSince.before(batch.getModified())) indexAssayBatch(queue, batch); diff --git a/audit/src/org/labkey/audit/AuditController.java b/audit/src/org/labkey/audit/AuditController.java index e44599c0749..acd22a9e9b5 100644 --- a/audit/src/org/labkey/audit/AuditController.java +++ b/audit/src/org/labkey/audit/AuditController.java @@ -383,11 +383,12 @@ public void validateForm(AuditTransactionForm form, Errors errors) public Object execute(AuditTransactionForm form, BindException errors) { List rowIds; - ContainerFilter cf = ContainerFilter.getContainerFilterByName(form.getContainerFilter(), getContainer(), getUser()); + User elevatedUser = ElevatedUser.ensureCanSeeAuditLogRole(getContainer(), getUser()); + ContainerFilter cf = ContainerFilter.getContainerFilterByName(form.getContainerFilter(), getContainer(), elevatedUser); if (form.isSampleType()) - rowIds = AuditLogImpl.get().getTransactionSampleIds(form.getTransactionAuditId(), ElevatedUser.ensureCanSeeAuditLogRole(getContainer(), getUser()), getContainer(), cf); + rowIds = AuditLogImpl.get().getTransactionSampleIds(form.getTransactionAuditId(), elevatedUser, getContainer(), cf); else - rowIds = AuditLogImpl.get().getTransactionSourceIds(form.getTransactionAuditId(), ElevatedUser.ensureCanSeeAuditLogRole(getContainer(), getUser()), getContainer(), cf); + rowIds = AuditLogImpl.get().getTransactionSourceIds(form.getTransactionAuditId(), elevatedUser, getContainer(), cf); ApiSimpleResponse response = new ApiSimpleResponse(); response.put("success", true); diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 0698a8e0f08..4c5cc386958 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -77,6 +77,7 @@ import org.labkey.api.data.TestSchema; import org.labkey.api.data.WorkbookContainerType; import org.labkey.api.data.dialect.BasePostgreSqlDialect; +import org.labkey.api.data.dialect.PostgreSqlService; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.data.dialect.SqlDialectManager; import org.labkey.api.data.dialect.SqlDialectRegistry; @@ -88,6 +89,7 @@ import org.labkey.api.files.FileBrowserConfigWriter; import org.labkey.api.files.FileContentService; import org.labkey.api.markdown.MarkdownService; +import org.labkey.api.mcp.McpService; import org.labkey.api.message.settings.MessageConfigService; import org.labkey.api.migration.DatabaseMigrationService; import org.labkey.api.module.FolderType; @@ -98,7 +100,6 @@ import org.labkey.api.module.SchemaUpdateType; import org.labkey.api.module.SpringModule; import org.labkey.api.module.Summary; -import org.labkey.api.mcp.McpService; import org.labkey.api.notification.EmailMessage; import org.labkey.api.notification.EmailService; import org.labkey.api.notification.NotificationMenuView; @@ -253,9 +254,9 @@ import org.labkey.core.login.DbLoginAuthenticationProvider; import org.labkey.core.login.DbLoginManager; import org.labkey.core.login.LoginController; +import org.labkey.core.mcp.McpServiceImpl; import org.labkey.core.metrics.SimpleMetricsServiceImpl; import org.labkey.core.metrics.WebSocketConnectionManager; -import org.labkey.core.mcp.McpServiceImpl; import org.labkey.core.notification.EmailPreferenceConfigServiceImpl; import org.labkey.core.notification.EmailPreferenceContainerListener; import org.labkey.core.notification.EmailPreferenceUserListener; @@ -560,11 +561,11 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) ScriptEngineManagerImpl.registerEncryptionMigrationHandler(); McpService.get().register(new CoreMcp()); + PostgreSqlService.setInstance(PostgreSqlDialectFactory::getLatestSupportedDialect); deleteTempFiles(); } - private void deleteTempFiles() { try diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 2f892250a6f..61764f3c6b1 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -886,10 +886,16 @@ public ActionURL getSystemMaintenanceURL() } @Override - public ActionURL getCspReportToURL(@NotNull String cspVersion) + public ActionURL getCspReportToURL() { - return new ActionURL(ContentSecurityPolicyReportToAction.class, ContainerManager.getRoot()) - .addParameter("cspVersion", cspVersion); + return new ActionURL(ContentSecurityPolicyReportToAction.class, ContainerManager.getRoot()); + } + + @Override + public ActionURL getAllowedExternalRedirectHostsURL() + { + return new ActionURL(AllowListAction.class, ContainerManager.getRoot()) + .addParameter("type", AllowListType.Redirect.name()); } public static ActionURL getDeprecatedFeaturesURL() @@ -12135,6 +12141,7 @@ protected boolean handleOneReport(JSONObject jsonObj, HttpServletRequest request if (!forwarded) { jsonObj.put("labkeyVersion", AppProps.getInstance().getReleaseVersion()); + jsonObj.put("cspVersion", ContentSecurityPolicyFilter.getCspVersion(cspReport.optString("disposition", null))); User user = getUser(); String email = null; // If the user is not logged in, we may still be able to snag the email address from our cookie @@ -12149,9 +12156,6 @@ protected boolean handleOneReport(JSONObject jsonObj, HttpServletRequest request jsonObj.put("ip", ipAddress); if (isNotBlank(userAgent) && !jsonObj.has("user_agent")) jsonObj.put("user_agent", userAgent); - String cspVersion = request.getParameter("cspVersion"); - if (null != cspVersion) - jsonObj.put("cspVersion", cspVersion); } var jsonStr = jsonObj.toString(2); diff --git a/core/src/org/labkey/core/admin/AllowListType.java b/core/src/org/labkey/core/admin/AllowListType.java index f2ebc545c3a..dc27efb07ea 100644 --- a/core/src/org/labkey/core/admin/AllowListType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -41,7 +41,7 @@ public HtmlString getDescription()

Add allowed hosts based on the server name or IP address, as they will be referenced in parameters - such as returnUrl. An asterisk (*) follow by a dot acts as a wild card that matches any leading + such as returnUrl. An asterisk (*) follow by a dot acts as a wildcard that matches any leading subdomain for that host. Examples: www.myexternalhost.com, *.myexternalhost.com, or 1.2.3.4

@@ -75,21 +75,21 @@ public void validateValueFormat(String host, BindException errors) { if (AUTHORITY_VALIDATOR.isValidAuthority(host)) { - // Validate wild card patterns + // Validate wildcard patterns int starCount = StringUtils.countMatches(host, '*'); if (starCount > 0) { if (starCount > 1) { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s has multiple wild card characters", host))); + errors.addError(new LabKeyError(String.format("Redirect host name %1$s has multiple wildcard characters", host))); } else if (!host.startsWith("*.")) { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s has an invalid wild card. The pattern must start with \"*.\".", host))); + errors.addError(new LabKeyError(String.format("Redirect host name %1$s has an invalid wildcard. The pattern must start with \"*.\".", host))); } else if (StringUtils.countMatches(host, '.') < 2) { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s with wild card is too short. The pattern must include at least two dots.", host))); + errors.addError(new LabKeyError(String.format("Redirect host name %1$s with wildcard is too short. The pattern must include at least two dots.", host))); } } } diff --git a/core/src/org/labkey/core/admin/sql/SqlScriptController.java b/core/src/org/labkey/core/admin/sql/SqlScriptController.java index 73f4e5678a5..a7868284993 100644 --- a/core/src/org/labkey/core/admin/sql/SqlScriptController.java +++ b/core/src/org/labkey/core/admin/sql/SqlScriptController.java @@ -1605,6 +1605,7 @@ public boolean handlePost(UpgradeCodeForm form, BindException errors) throws Exc { LOG.info("Executing {}.{}(ModuleContext moduleContext)", _method.getDeclaringClass().getSimpleName(), _method.getName()); _method.invoke(_code, _ctx); + LOG.info("Finished executing {}.{}(ModuleContext moduleContext)", _method.getDeclaringClass().getSimpleName(), _method.getName()); return true; } diff --git a/core/src/org/labkey/core/dialect/PostgreSqlDialectFactory.java b/core/src/org/labkey/core/dialect/PostgreSqlDialectFactory.java index 5e3fa1914f4..3a8a4845799 100644 --- a/core/src/org/labkey/core/dialect/PostgreSqlDialectFactory.java +++ b/core/src/org/labkey/core/dialect/PostgreSqlDialectFactory.java @@ -24,6 +24,7 @@ import org.junit.Assert; import org.junit.Test; import org.labkey.api.data.dialect.AbstractDialectRetrievalTestCase; +import org.labkey.api.data.dialect.BasePostgreSqlDialect; import org.labkey.api.data.dialect.DatabaseNotSupportedException; import org.labkey.api.data.dialect.JdbcHelperTest; import org.labkey.api.data.dialect.PostgreSqlServerType; @@ -145,6 +146,11 @@ public static PostgreSql_13_Dialect getOldestSupportedDialect() return new PostgreSql_13_Dialect(); } + public static BasePostgreSqlDialect getLatestSupportedDialect() + { + return new PostgreSql_18_Dialect(); + } + public static class DialectRetrievalTestCase extends AbstractDialectRetrievalTestCase { @Override diff --git a/core/src/org/labkey/core/user/UserController.java b/core/src/org/labkey/core/user/UserController.java index fdf3d452105..6cf82cd7c7b 100644 --- a/core/src/org/labkey/core/user/UserController.java +++ b/core/src/org/labkey/core/user/UserController.java @@ -98,6 +98,7 @@ import org.labkey.api.security.ValidEmail.InvalidEmailException; import org.labkey.api.security.impersonation.GroupImpersonationContextFactory; import org.labkey.api.security.impersonation.RoleImpersonationContextFactory; +import org.labkey.api.security.impersonation.RoleImpersonationContextFactory.RoleImpersonationContext; import org.labkey.api.security.impersonation.UnauthorizedImpersonationException; import org.labkey.api.security.impersonation.UserImpersonationContextFactory; import org.labkey.api.security.permissions.AbstractActionPermissionTest; @@ -105,8 +106,8 @@ import org.labkey.api.security.permissions.AdminOperationsPermission; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.CanAccessLockedProjectsPermission; -import org.labkey.api.security.permissions.CanImpersonateSiteRolesPermission; import org.labkey.api.security.permissions.DeleteUserPermission; +import org.labkey.api.security.permissions.ImpersonatePermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdateUserPermission; @@ -896,7 +897,7 @@ private boolean isProjectAdmin(User user) @RequiresPermission(AdminPermission.class) - public class ShowUserHistoryAction extends SimpleViewAction + public class ShowUserHistoryAction extends SimpleViewAction { @Override public void checkPermissions() throws UnauthorizedException @@ -2786,7 +2787,7 @@ public ApiResponse execute(GetUsersForm form, BindException errors) } - @RequiresPermission(AdminPermission.class) + @RequiresNoPermission // getValidImpersonationUsers() does all permission checking public static class GetImpersonationUsersAction extends MutatingApiAction { @Override @@ -2795,9 +2796,7 @@ public ApiResponse execute(Object object, BindException errors) ApiSimpleResponse response = new ApiSimpleResponse(); User currentUser = getUser(); - Container project = currentUser.hasRootAdminPermission() ? null : getContainer().getProject(); - Collection users = UserImpersonationContextFactory.getValidImpersonationUsers(project, getUser()); - + Collection users = UserImpersonationContextFactory.getValidImpersonationUsers(getContainer().getProject(), currentUser); Collection> responseUsers = new LinkedList<>(); for (User user : users) @@ -2872,9 +2871,8 @@ public ApiResponse execute(FORM form, BindException errors) public abstract @Nullable String impersonate(FORM form); } - - @RequiresPermission(AdminPermission.class) - public class ImpersonateUserAction extends ImpersonateApiAction + @RequiresNoPermission + public static class ImpersonateUserAction extends ImpersonateApiAction { @Override public @Nullable String impersonate(ImpersonateUserForm form) @@ -2912,8 +2910,8 @@ else if (form.getEmail() != null) } } - - @RequiresPermission(AdminPermission.class) + // getValidImpersonationGroups() checks permissions and returns empty for user who can't impersonate + @RequiresNoPermission public static class GetImpersonationGroupsAction extends MutatingApiAction { @Override @@ -2937,7 +2935,6 @@ public ApiResponse execute(Object object, BindException errors) } } - public static class ImpersonateGroupForm extends ReturnUrlForm { private Integer _groupId = null; @@ -2954,11 +2951,10 @@ public void setGroupId(Integer groupId) } } - // TODO: Better instructions // TODO: Messages for no groups, no users - @RequiresPermission(AdminPermission.class) - public class ImpersonateGroupAction extends ImpersonateApiAction + @RequiresNoPermission + public static class ImpersonateGroupAction extends ImpersonateApiAction { @Override public @Nullable String impersonate(ImpersonateGroupForm form) @@ -2991,15 +2987,16 @@ public class ImpersonateGroupAction extends ImpersonateApiAction + public static class GetImpersonationRolesAction extends MutatingApiAction { @Override public ApiResponse execute(Object object, BindException errors) { - PermissionsContext context = authorizeImpersonateRoles(); - Set impersonationRoles = context.isImpersonating() ? context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : Collections.emptySet(); + PermissionsContext context = getUser().getPermissionsContext(); + Set currentRoles = context.isImpersonating() && context instanceof RoleImpersonationContext ? + context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : + Collections.emptySet(); User user = context.isImpersonating() ? context.getAdminUser() : getUser(); ApiSimpleResponse response = new ApiSimpleResponse(); @@ -3009,7 +3006,7 @@ public ApiResponse execute(Object object, BindException errors) map.put("displayName", role.getDisplayName()); map.put("roleName", role.getUniqueName()); map.put("hasRead", role.getPermissions().contains(ReadPermission.class)); - map.put("selected", impersonationRoles.contains(role)); + map.put("selected", currentRoles.contains(role)); return map; }) .toList(); @@ -3020,25 +3017,6 @@ public ApiResponse execute(Object object, BindException errors) } } - - private PermissionsContext authorizeImpersonateRoles() - { - User user = getUser(); - PermissionsContext context = user.getPermissionsContext(); - - if (context.isImpersonating()) - user = context.getAdminUser(); - - if (getContainer().isRoot() && user.hasRootPermission(CanImpersonateSiteRolesPermission.class)) - return context; - - if (!getContainer().hasPermission(user, AdminPermission.class)) - throw new UnauthorizedException(); - - return context; - } - - public static class ImpersonateRolesForm extends ReturnUrlForm { private String[] _roleNames; @@ -3048,23 +3026,26 @@ public String[] getRoleNames() return _roleNames; } + @SuppressWarnings("unused") public void setRoleNames(String[] roleNames) { _roleNames = roleNames; } } - - // Permissions are checked in impersonate() to let an admin adjust an existing impersonation + // Permissions are checked by the RoleImpersonationFactory. This lets impersonators adjust an existing impersonation + // and impersonating troubleshooters impersonate in projects and folders. @RequiresNoPermission - public class ImpersonateRolesAction extends ImpersonateApiAction + public static class ImpersonateRolesAction extends ImpersonateApiAction { @Nullable @Override public String impersonate(ImpersonateRolesForm form) { - PermissionsContext context = authorizeImpersonateRoles(); - Set currentImpersonationRoles = context.isImpersonating() ? context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : Collections.emptySet(); + PermissionsContext context = getUser().getPermissionsContext(); + Set currentImpersonationRoles = context.isImpersonating() && context instanceof RoleImpersonationContext ? + context.getAssignedRoles(getUser(), getContainer()).collect(Collectors.toSet()) : + Collections.emptySet(); String[] roleNames = form.getRoleNames(); @@ -3232,6 +3213,15 @@ public void testActionPermissions() UserController controller = new UserController(); + assertForNoPermission(user, + new GetImpersonationUsersAction(), + new ImpersonateUserAction(), + new GetImpersonationGroupsAction(), + new ImpersonateGroupAction(), + new GetImpersonationRolesAction(), + new ImpersonateRolesAction() + ); + // @RequiresPermission(ReadPermission.class) assertForReadPermission(user, false, new BeginAction(), @@ -3241,15 +3231,9 @@ public void testActionPermissions() // @RequiresPermission(AdminPermission.class) assertForAdminPermission(user, - controller.new ShowUsersAction(), + controller.new ShowUsersAction() //TODO controller.new ShowUserHistoryAction(), //TODO controller.new UserAccessAction(), - new GetImpersonationUsersAction(), - controller.new ImpersonateUserAction(), - new GetImpersonationGroupsAction(), - controller.new ImpersonateGroupAction() -// controller.new GetImpersonationRolesAction() Annotated as "no permission", to allow impersonation adjustments -// controller.new ImpersonateRolesAction() Annotated as "no permission", to allow impersonation adjustments ); // @RequiresPermission(UserManagementPermission.class) diff --git a/core/test/src/org/labkey/test/tests/upgrade/EncryptionKeyUpgradeTest.java b/core/test/src/org/labkey/test/tests/upgrade/EncryptionKeyUpgradeTest.java index 65f403d7684..e5f985791fc 100644 --- a/core/test/src/org/labkey/test/tests/upgrade/EncryptionKeyUpgradeTest.java +++ b/core/test/src/org/labkey/test/tests/upgrade/EncryptionKeyUpgradeTest.java @@ -3,6 +3,8 @@ import org.junit.Test; import org.junit.experimental.categories.Category; import org.labkey.test.Locators; +import org.labkey.test.WebTestHelper; +import org.labkey.test.WebTestHelper.DatabaseType; import org.labkey.test.pages.ConfigureReportsAndScriptsPage; import org.labkey.test.pages.ConfigureReportsAndScriptsPage.EngineType; import org.labkey.test.pages.admin.RConfigurationPage; @@ -42,10 +44,13 @@ protected void doSetup() .setEngineOverrides(DUMMY_R_SERVE, DUMMY_R_SERVE) .save(); - // Set StatusCake api key - EditUpgradeMessagePage.beginAt(this) - .setStatusCakeApiKey("password") - .save(); + if (WebTestHelper.getDatabaseType() == DatabaseType.PostgreSQL) + { + // Set StatusCake api key + EditUpgradeMessagePage.beginAt(this) + .setStatusCakeApiKey("password") + .save(); + } } @Test @@ -64,9 +69,12 @@ public void testDummyRemoteREngine() @Test public void testStatusCakeApiKey() { - // Just loading this page can trigger an error if there was a problem with the encryption - assertEquals("StatusCake API key input should be present but blank", - "", EditUpgradeMessagePage.beginAt(this).getStatusCakeApiKey()); + if (WebTestHelper.getDatabaseType() == DatabaseType.PostgreSQL) + { + // Just loading this page can trigger an error if there was a problem with the encryption + assertEquals("StatusCake API key input should be present but blank", + "", EditUpgradeMessagePage.beginAt(this, null).getStatusCakeApiKey()); // Use the root container in case the '_mothership' project doesn't exist + } } @Override diff --git a/core/webapp/Impersonate.js b/core/webapp/Impersonate.js index 653b4accf6d..1bd300291b8 100644 --- a/core/webapp/Impersonate.js +++ b/core/webapp/Impersonate.js @@ -35,7 +35,7 @@ Ext4.define('LABKEY.Security.ImpersonateUser', { getPanel: function(){ var instructions = LABKEY.Security.currentUser.isRootAdmin ? - "As a site or application administrator, you can impersonate any user on the site." + + "As a site administrator, application administrator, or impersonating troubleshooter, you can impersonate any user on the site." + (!LABKEY.Security.currentUser.isSystemAdmin ? " While impersonating you will not inherit the user's " + "site-level roles (e.g., Site Administrator, Developer)." : "") : @@ -191,7 +191,7 @@ Ext4.define('LABKEY.Security.ImpersonateGroup', { getPanel: function(){ var instructions = LABKEY.Security.currentUser.isRootAdmin ? - "As a site or application administrator, you can impersonate any site or project group." : + "As a site administrator, application administrator, or impersonating troubleshooter, you can impersonate any site or project group." : "As a project administrator, you can impersonate any project group in this project or any site group in which you're member. While impersonating you will be restricted to this project."; var divContainer = Ext4.create('Ext.container.Container', { @@ -315,7 +315,7 @@ Ext4.define('LABKEY.Security.ImpersonateRoles', { getPanel: function(){ var instructions = LABKEY.Security.currentUser.canImpersonateSiteRoles ? - "As a site administrator, application administrator, or impersonating troubleshooter you can impersonate one or more security roles. While impersonating you will have access to " + + "As a site administrator, application administrator, or impersonating troubleshooter, you can impersonate one or more security roles. While impersonating you will have access to " + "the entire site, limited to the permissions provided by the selected roles(s)." : "As a project administrator, you can impersonate one or more security roles. While impersonating you will be restricted to this project."; diff --git a/experiment/resources/schemas/dbscripts/sqlserver/exp-26.004-26.005.sql b/experiment/resources/schemas/dbscripts/sqlserver/exp-26.004-26.005.sql new file mode 100644 index 00000000000..b6f3e71a4b5 --- /dev/null +++ b/experiment/resources/schemas/dbscripts/sqlserver/exp-26.004-26.005.sql @@ -0,0 +1,2 @@ +-- SQL Server only +EXEC core.executeJavaUpgradeCode 'shortenAllStorageNames'; diff --git a/experiment/src/client/test/integration/AssayImportRunAction.ispec.ts b/experiment/src/client/test/integration/AssayImportRunAction.ispec.ts index f38134375c1..04f1105f12e 100644 --- a/experiment/src/client/test/integration/AssayImportRunAction.ispec.ts +++ b/experiment/src/client/test/integration/AssayImportRunAction.ispec.ts @@ -118,7 +118,7 @@ async function verifyPropertiesFilesOnServer( // Note: this is a hack to allow us to make requests to the webdav controller. The IntegrationTestServer // request method uses ActionURL to generate URLS, but webdav URLs are not ActionURLs, so we need to // override the AgentProvider to generate the proper webdav URL. - url = `/_webdav/${requestOptions.containerPath}/%40files/assaydata?method=JSON`; + url = `${LABKEY.contextPath}/_webdav/${requestOptions.containerPath}/%40files/assaydata?method=JSON`; return agent.get(url); }, requestOptions diff --git a/experiment/src/org/labkey/experiment/DataClassMigrationSchemaHandler.java b/experiment/src/org/labkey/experiment/DataClassMigrationSchemaHandler.java index 0069378ea20..d9071d11b14 100644 --- a/experiment/src/org/labkey/experiment/DataClassMigrationSchemaHandler.java +++ b/experiment/src/org/labkey/experiment/DataClassMigrationSchemaHandler.java @@ -178,11 +178,12 @@ public void afterSchema(DatabaseMigrationConfiguration configuration, DbSchema s DbScope sourceScope = configuration.getSourceScope(); DbScope targetScope = configuration.getTargetScope(); - DbSchema biologicsSourceSchema = sourceScope.getSchema("biologics", DbSchemaType.Migration); - DbSchema biologicsTargetSchema = targetScope.getSchema("biologics", DbSchemaType.Module); - if (biologicsSourceSchema.existsInDatabase() && biologicsTargetSchema.existsInDatabase()) + if (sourceScope.getSchemaNames().contains("biologics") && targetScope.getSchemaNames().contains("biologics")) { + DbSchema biologicsSourceSchema = sourceScope.getSchema("biologics", DbSchemaType.Migration); + DbSchema biologicsTargetSchema = targetScope.getSchema("biologics", DbSchemaType.Module); + TableInfo sourceTable = biologicsSourceSchema.getTable("SequenceIdentity"); TableInfo targetTable = biologicsTargetSchema.getTable("SequenceIdentity"); diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 52c07a06836..5d452027778 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -205,7 +205,7 @@ public String getName() @Override public Double getSchemaVersion() { - return 26.004; + return 26.005; } @Nullable @@ -660,6 +660,17 @@ SELECT COUNT(DISTINCT DD.DomainURI) FROM JOIN exp.DomainDescriptor DD on PD.domainID = DD.domainId WHERE DD.domainUri LIKE ? AND D.rangeURI = ?""", "urn:lsid:%:" + ExpProtocol.AssayDomainTypes.Result.getPrefix() + ".%", PropertyType.FILE_LINK.getTypeUri()).getObject(Long.class)); + // metric to count the number of Luminex and Standard assay runs that were imported with > 1 data file + assayMetrics.put("assayRunsWithMultipleInputFiles", new SqlSelector(schema, """ + SELECT COUNT(*) FROM ( + SELECT sourceapplicationid, COUNT(*) AS count FROM exp.data + WHERE lsid NOT LIKE '%:RelatedFile.%' AND sourceapplicationid IN ( + SELECT rowid FROM exp.protocolapplication + WHERE lsid LIKE '%:SimpleProtocol.CoreStep' AND (protocollsid LIKE '%:LuminexAssayProtocol.%' OR protocollsid LIKE '%:GeneralAssayProtocol.%') + ) + GROUP BY sourceapplicationid + ) x WHERE count > 1""").getObject(Long.class)); + Map sampleLookupCountMetrics = new HashMap<>(); SQLFragment baseAssaySampleLookupSQL = new SQLFragment("SELECT COUNT(*) FROM exp.propertydescriptor WHERE (lookupschema = 'samples' OR (lookupschema = 'exp' AND lookupquery = 'Materials')) AND propertyuri LIKE ?"); diff --git a/experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java b/experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java index 8c4a644d3cb..e693224a7ec 100644 --- a/experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java +++ b/experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java @@ -15,6 +15,8 @@ */ package org.labkey.experiment; +import org.apache.commons.collections4.MultiValuedMap; +import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; @@ -25,11 +27,15 @@ import org.labkey.api.audit.SampleTimelineAuditEvent; import org.labkey.api.audit.TransactionAuditProvider; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.CsvSet; +import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.data.ColumnInfo; +import org.labkey.api.data.CompareType; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbScope; +import org.labkey.api.data.DbScope.Transaction; import org.labkey.api.data.DeferredUpgrade; import org.labkey.api.data.JdbcType; import org.labkey.api.data.Parameter; @@ -39,11 +45,16 @@ import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SchemaTableInfo; import org.labkey.api.data.Selector; +import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.SqlSelector; +import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; import org.labkey.api.data.UpgradeCode; +import org.labkey.api.data.dialect.BasePostgreSqlDialect; +import org.labkey.api.data.dialect.PostgreSqlService; +import org.labkey.api.exp.OntologyManager; import org.labkey.api.exp.PropertyDescriptor; import org.labkey.api.exp.api.ExpSampleType; import org.labkey.api.exp.api.ExperimentService; @@ -64,6 +75,8 @@ import org.labkey.api.security.User; import org.labkey.api.security.roles.SiteAdminRole; import org.labkey.api.settings.AppProps; +import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.StringUtilsLabKey; import org.labkey.api.util.logging.LogHelper; import org.labkey.experiment.api.DataClass; import org.labkey.experiment.api.DataClassDomainKind; @@ -73,11 +86,14 @@ import org.labkey.experiment.api.MaterialSource; import org.labkey.experiment.api.property.DomainImpl; import org.labkey.experiment.api.property.DomainPropertyImpl; +import org.labkey.experiment.api.property.StorageNameGenerator; import org.labkey.experiment.api.property.StorageProvisionerImpl; +import java.nio.charset.StandardCharsets; import java.sql.Connection; import java.sql.SQLException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -153,7 +169,7 @@ public static void upgradeAmountsAndUnits(ModuleContext context) DbScope scope = ExperimentService.get().getSchema().getScope(); LimitedUser admin = new LimitedUser(context.getUpgradeUser(), SiteAdminRole.class); - try (DbScope.Transaction transaction = scope.ensureTransaction()) + try (Transaction transaction = scope.ensureTransaction()) { // create a single transaction event at the root container for use in tying all updates together TransactionAuditProvider.TransactionAuditEvent transactionEvent = AbstractQueryUpdateService.createTransactionAuditEvent(ContainerManager.getRoot(), QueryService.AuditAction.UPDATE); @@ -174,7 +190,6 @@ public static void upgradeAmountsAndUnits(ModuleContext context) } ExperimentService.get().clearCaches(); } - } private static void getAmountAndUnitUpdates(Map sampleMap, Parameter unitsCol, Set amountCols, Unit currentDisplayUnit, Map oldDataMap, Map newDataMap, Map sampleCounts, boolean aliquotFields) @@ -366,7 +381,7 @@ public static void dropProvisionedSampleTypeLsidColumn(ModuleContext context) if (context.isNewInstall()) return; - try (DbScope.Transaction tx = ExperimentService.get().ensureTransaction()) + try (Transaction tx = ExperimentService.get().ensureTransaction()) { // Process all sample types across all containers TableInfo sampleTypeTable = ExperimentServiceImpl.get().getTinfoSampleType(); @@ -500,7 +515,7 @@ public static void fixContainerForMovedSampleFiles(ModuleContext context) if (context.isNewInstall()) return; - try (DbScope.Transaction tx = ExperimentService.get().ensureTransaction()) + try (Transaction tx = ExperimentService.get().ensureTransaction()) { FileContentService service = FileContentService.get(); if (service == null) @@ -525,7 +540,7 @@ public static void addRowIdToProvisionedDataClassTables(ModuleContext context) if (context.isNewInstall()) return; - try (DbScope.Transaction tx = ExperimentService.get().ensureTransaction()) + try (Transaction tx = ExperimentService.get().ensureTransaction()) { TableInfo source = ExperimentServiceImpl.get().getTinfoDataClass(); new TableSelector(source, null, null).stream(DataClass.class) @@ -656,5 +671,149 @@ private static void fillRowId(ExpDataClassImpl dc, Domain domain, DbScope scope) LOG.info("DataClass '{}' ({}) populated 'rowId' column, count={}", dc.getName(), dc.getRowId(), count); } + record DomainRecord(Container container, int domainId, String name, String storageSchemaName, String storageTableName) + { + String fullName() + { + return storageSchemaName + "." + storageTableName; + } + } + + record Property(int domainId, int propertyId, String domainName, String name, String storageSchemaName, String storageTableName, String storageColumnName) + { + String fullName() + { + // Have to bracket storage column name since it could have special characters (like dots) + return storageSchemaName + "." + storageTableName + "." + bracketIt(storageColumnName); + } + + // Bracket name and escape any internal ending brackets + private String bracketIt(String name) + { + return "[" + name.replace("]", "]]") + "]"; + } + } + + /** + * Called from exp-26.004-26.005.sql, on SQL Server only + * GitHub Issue 869: Long table/column names cause SQL Server migration to fail + * Query all table & column storage names and rename the ones that are too long for PostgreSQL + * TODO: When this upgrade code is removed, get rid of the StorageProvisionerImpl.makeTableName() method it uses. + */ + @SuppressWarnings("unused") + public static void shortenAllStorageNames(ModuleContext context) + { + if (context.isNewInstall()) + return; + + // The PostgreSQL dialect knows which names are too long + BasePostgreSqlDialect dialect = PostgreSqlService.get().getDialect(); + DbScope scope = DbScope.getLabKeyScope(); + SqlExecutor executor = new SqlExecutor(scope); + // Stream all the storage table names and rename the ones that are too long for PostgreSQL. The filtering must + // be done in code by the dialect; SQL Server has BYTELENGTH(), but that function returns values that are not + // consistent with our dialect check. Also, it looks like the function's behavior changed starting in SS 2019. + TableInfo tinfoDomainDescriptor = OntologyManager.getTinfoDomainDescriptor(); + SimpleFilter filter = new SimpleFilter(FieldKey.fromString("StorageSchemaName"), null, CompareType.NONBLANK); + filter.addCondition(FieldKey.fromString("StorageTableName"), null, CompareType.NONBLANK); + + new TableSelector(tinfoDomainDescriptor, new CsvSet("Container, DomainId, Name, StorageSchemaName, StorageTableName"), filter, null) + .setJdbcCaching(false) + .stream(DomainRecord.class) + .filter(domain -> dialect.isIdentifierTooLong(domain.storageTableName())) + .forEach(domain -> { + String oldName = domain.fullName(); + String newName = StorageProvisionerImpl.get().makeTableName(dialect, domain.container(), domain.domainId(), domain.name()); + + try (Transaction transaction = scope.beginTransaction()) + { + executor.execute(new SQLFragment("EXEC sp_rename ?, ?").add(oldName).add(newName)); + Table.update(null, tinfoDomainDescriptor, PageFlowUtil.map("StorageTableName", newName), domain.domainId()); + transaction.commit(); + } + + LOG.info(" Table \"{}\" renamed to \"{}\" ({} bytes)", oldName, newName, newName.getBytes(StandardCharsets.UTF_8).length); + }); + + List badTableNames = new TableSelector(tinfoDomainDescriptor, new CsvSet("StorageTableName"), filter, null) + .setJdbcCaching(false) + .stream(String.class) + .filter(dialect::isIdentifierTooLong) + .toList(); + + if (!badTableNames.isEmpty()) + LOG.error("Some storage table names are still too long!! {}", badTableNames); + + // Collect all the domains that have one or more storage columns names that are too long for PostgreSQL + TableInfo tinfoPropertyDomain = OntologyManager.getTinfoPropertyDomain(); + TableInfo tinfoPropertyDescriptor = OntologyManager.getTinfoPropertyDescriptor(); + SQLFragment sql = new SQLFragment("SELECT dd.DomainId, dd.Name AS DomainName, px.PropertyId, StorageSchemaName, StorageTableName, StorageColumnName, px.Name FROM ") + .append(tinfoDomainDescriptor, "dd") + .append(" INNER JOIN ") + .append(tinfoPropertyDomain, "pd") + .append(" ON dd.DomainId = pd.DomainId INNER JOIN ") + .append(tinfoPropertyDescriptor, "px") + .append(" ON pd.PropertyId = px.PropertyId ") + .append("WHERE StorageSchemaName IS NOT NULL AND StorageTableName IS NOT NULL AND StorageColumnName IS NOT NULL"); + + MultiValuedMap badDomainMap = new SqlSelector(scope, sql) + .setJdbcCaching(false) + .stream(Property.class) + .filter(property -> dialect.isIdentifierTooLong(property.storageColumnName())) + .collect(LabKeyCollectors.toMultiValuedMap( + property -> new DomainRecord(null, property.domainId(), property.domainName(), property.storageSchemaName(), property.storageTableName()), + property -> property, + ArrayListValuedHashMap::new) + ); + + if (!badDomainMap.isEmpty()) + LOG.info(" Found {} with storage column names that are too long for PostgreSQL:", StringUtilsLabKey.pluralize(badDomainMap.keySet().size(), "domain")); + + // Now enumerate the bad domains and rename their bad storage columns using the PostgreSQL truncation rules + badDomainMap.keySet() + .forEach(domain -> { + Collection badColumns = badDomainMap.get(domain); + List badColumnNames = badColumns.stream().map(Property::storageColumnName).toList(); + + // First, populate a new StorageNameGenerator with all the "good" names in this domain so we don't + // accidentally try to re-use one of them + StorageNameGenerator nameGenerator = new StorageNameGenerator(dialect); + SQLFragment domainSql = new SQLFragment("SELECT StorageColumnName FROM ") + .append(tinfoPropertyDomain, "pd") + .append(" INNER JOIN ") + .append(tinfoPropertyDescriptor, "px") + .append(" ON pd.PropertyId = px.PropertyId ") + .append("WHERE DomainId = ? AND StorageColumnName NOT ") + .add(domain.domainId()) + .appendInClause(badColumnNames, scope.getSqlDialect()); + new SqlSelector(scope, domainSql).forEach(String.class, nameGenerator::claimName); + + LOG.info(" Renaming {} in table \"{}\"", StringUtilsLabKey.pluralize(badColumns.size(), "column"), domain.fullName()); + + // Now use that StorageNameGenerator to create new names. Rename the column and update the PropertyDescriptor table. + badColumns.forEach(property -> { + String oldName = property.fullName(); + String newName = nameGenerator.generateColumnName(property.name()); // No need to bracket or quote or escape: JDBC parameter takes care of all special characters + + try (Transaction transaction = scope.beginTransaction()) + { + executor.execute(new SQLFragment("EXEC sp_rename ?, ?, 'COLUMN'").add(oldName).add(newName)); + Table.update(null, tinfoPropertyDescriptor, PageFlowUtil.map("StorageColumnName", newName), property.propertyId()); + transaction.commit(); + } + + LOG.info(" Column \"{}\" renamed to \"{}\" ({} bytes)", oldName, newName, newName.getBytes(StandardCharsets.UTF_8).length); + }); + }); + + List badColumnNames = new TableSelector(tinfoPropertyDescriptor, new CsvSet("StorageColumnName"), new SimpleFilter(FieldKey.fromString("StorageColumnName"), null, CompareType.NONBLANK), null) + .setJdbcCaching(false) + .stream(String.class) + .filter(dialect::isIdentifierTooLong) + .toList(); + + if (!badColumnNames.isEmpty()) + LOG.error("Some storage column names are still too long!! {}", badColumnNames); + } } diff --git a/experiment/src/org/labkey/experiment/api/ExpProtocolImpl.java b/experiment/src/org/labkey/experiment/api/ExpProtocolImpl.java index 82f611ce76d..6b8a209c0bf 100644 --- a/experiment/src/org/labkey/experiment/api/ExpProtocolImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpProtocolImpl.java @@ -21,7 +21,6 @@ import org.labkey.api.assay.AssayProvider; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; -import org.labkey.api.data.Filter; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.SqlSelector; @@ -329,9 +328,12 @@ public List getChildProtocols() } @Override - public List getBatches() + public List getBatches(@Nullable Container c) { - Filter filter = new SimpleFilter(FieldKey.fromParts("BatchProtocolId"), getRowId()); + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("BatchProtocolId"), getRowId()); + // GitHub Issue 895: add param option to get just the assay batches for a specific container + if (c != null) + filter.addCondition(FieldKey.fromParts("container"), c); return ExpExperimentImpl.fromExperiments(new TableSelector(ExperimentServiceImpl.get().getTinfoExperiment(), filter, null).getArray(Experiment.class)); } diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index 0758c8a156b..401357afcc1 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -4528,7 +4528,7 @@ public void deleteProtocolByRowIds(Container c, User user, @Nullable String audi for (ExpProtocol protocolToDelete : expProtocols) { - for (ExpExperiment batch : protocolToDelete.getBatches()) + for (ExpExperiment batch : protocolToDelete.getBatches(null)) { batch.delete(user); } diff --git a/experiment/src/org/labkey/experiment/api/property/StorageNameGenerator.java b/experiment/src/org/labkey/experiment/api/property/StorageNameGenerator.java index 16e82fccc48..d6bdbf9c596 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageNameGenerator.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageNameGenerator.java @@ -6,6 +6,7 @@ import org.junit.Test; import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.data.DbScope; +import org.labkey.api.data.dialect.PostgreSqlService; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.exp.OntologyManager; import org.labkey.api.util.StringUtilsLabKey; @@ -32,7 +33,8 @@ public class StorageNameGenerator public StorageNameGenerator(@NotNull SqlDialect dialect) { - _dialect = dialect; + // GitHub Issue 869: Create SQL Server storage names using PostgreSQL's rules to ensure all tables and columns can migrate + _dialect = dialect.isSqlServer() ? PostgreSqlService.get().getDialect() : dialect; } public String claimName(String name) @@ -75,7 +77,7 @@ private String generateName(String candidateName, int reserved) ret = legalName + i; } - if (_dialect.isIdentifierTooLong(ret)) + if (isIdentifierTooLong(ret)) throw new IllegalStateException("generateName() produced a name that was too long for " + _dialect.getProductName() + ": \"" + ret + "\" was generated from \"" + candidateName + "\""); if (ret.length() > 255) @@ -84,6 +86,11 @@ private String generateName(String candidateName, int reserved) return claimName(ret); } + private boolean isIdentifierTooLong(String candidate) + { + return _dialect.isIdentifierTooLong(candidate); + } + public static class TestCase extends Assert { @Test @@ -99,11 +106,11 @@ public void testGenerateName() testCandidates(255, dialect, StringUtilsLabKey::generateSpecialCharacterString); // Test that the same string over and over again generates a unique name - testCandidates(255, dialect, i -> "kumquat"); + testCandidates(255, dialect, _ -> "kumquat"); // Same, but test case sensitivity testCandidates(255, dialect, i -> i % 2 == 0 ? "kumquat" : "KUMQUAT"); String randomString = StringUtilsLabKey.generateSpecialCharacterString(255); - testCandidates(255, dialect, i -> randomString); + testCandidates(255, dialect, _ -> randomString); } private void testCandidates(int count, SqlDialect dialect, Function candidateSupplier) @@ -117,7 +124,7 @@ private void testCandidates(int count, SqlDialect dialect, Function kind, Domain domain) { - String rawTableName = String.format("c%sd%s_%s", domain.getContainer().getRowId(), domain.getTypeId(), domain.getName()); - SqlDialect dialect = kind.getScope().getSqlDialect(); + return makeTableName(kind.getScope().getSqlDialect(), domain.getContainer(), domain.getTypeId(), domain.getName()); + } + + // Needed by ExperimentUpgradeCode.shortenAllStorageNames(). When that code is removed, combine this with above variant. + public String makeTableName(SqlDialect dialect, Container c, int typeId, String domainName) + { + String rawTableName = String.format("c%sd%s_%s", c.getRowId(), typeId, domainName); return new StorageNameGenerator(dialect).generateTableName(rawTableName); } diff --git a/list/src/org/labkey/list/controllers/ListController.java b/list/src/org/labkey/list/controllers/ListController.java index 44cda4aa4d5..2a5b90e4e8f 100644 --- a/list/src/org/labkey/list/controllers/ListController.java +++ b/list/src/org/labkey/list/controllers/ListController.java @@ -51,6 +51,7 @@ import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DataRegionSelection; +import org.labkey.api.data.DbScope; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.TableInfo; @@ -116,6 +117,7 @@ import org.labkey.list.model.ListDomainKindProperties; import org.labkey.list.model.ListManager; import org.labkey.list.model.ListManagerSchema; +import org.labkey.list.model.ListSchema; import org.labkey.list.model.ListWriter; import org.labkey.list.view.ListDefinitionForm; import org.labkey.list.view.ListItemAttachmentParent; @@ -425,6 +427,103 @@ public List> getListContainerMap() } } + @RequiresPermission(AdminPermission.class) + public static class TruncateListDataAction extends ConfirmAction + { + private boolean canTruncate(Container listContainer, int listId) + { + ListDef listDef = ListManager.get().getList(listContainer, listId); + ListDefinitionImpl list = ListDefinitionImpl.of(listDef); + + if (list == null || !list.getAllowDelete()) + return false; + + return list.getContainer().hasPermission(getUser(), AdminPermission.class); + } + + @Override + public String getConfirmText() + { + return "Confirm Delete All Data"; + } + + @Override + public void validateCommand(ListDeletionForm form, Errors errors) + { + Container currentContainer = getContainer(); + List errorMessages = new ArrayList<>(); + Collection listIDs; + if (form.getListIds() != null) + listIDs = form.getListIds(); + else + listIDs = DataRegionSelection.getSelected(form.getViewContext(), true); + + for (Pair pair : getListIdContainerPairs(listIDs, currentContainer, errorMessages)) + { + var listId = pair.first; + var listContainer = pair.second; + + if (canTruncate(listContainer, listId)) + { + form.getListContainerMap().add(pair); + } + else + errorMessages.add(String.format("You do not have permission to delete data for list %s in container %s", listId, listContainer.getName())); + } + + if (!errorMessages.isEmpty()) + errors.reject(ERROR_MSG, StringUtils.join(errorMessages, "\n")); + + if (form.getListContainerMap().isEmpty()) + errors.reject(ERROR_MSG, "You must specify a list or lists to delete data from."); + } + + @Override + public ModelAndView getConfirmView(ListDeletionForm form, BindException errors) + { + if (getPageConfig().getTitle() == null) + setTitle("Confirm Delete All Data"); + return new JspView<>("/org/labkey/list/view/truncateListData.jsp", form, errors); + } + + @Override + public boolean handlePost(ListDeletionForm form, BindException errors) + { + Container containerDataToDelete = getContainer(); + try (DbScope.Transaction transaction = ListSchema.getInstance().getSchema().getScope().ensureTransaction()) + { + for (Pair pair : form.getListContainerMap()) + { + Container listDefContainer = pair.second; + ListDefinition listDef = ListService.get().getList(listDefContainer, pair.first); + if (null != listDef) + { + try + { + TableInfo table = listDef.getTable(getUser(), listDefContainer); + if (table != null && table.getUpdateService() != null) + table.getUpdateService().truncateRows(getUser(), containerDataToDelete, null, null); + } + catch (Exception e) + { + errors.reject(ERROR_MSG, "Error deleting data from list '" + listDef.getName() + "': " + e.getMessage()); + return false; + } + } + } + + transaction.commit(); + } + + return !errors.hasErrors(); + } + + @Override @NotNull + public URLHelper getSuccessURL(ListDeletionForm form) + { + return form.getReturnUrlHelper(getBeginURL(getContainer())); + } + } @RequiresPermission(ReadPermission.class) public class GridAction extends SimpleViewAction @@ -958,10 +1057,11 @@ public Pair getAttachment(ListAttachmentForm form) if (listDef == null) throw new NotFoundException("List does not exist in this container"); - if (!listDef.hasListItemForEntityId(form.getEntityId(), getUser())) + Container dataContainer = listDef.getListItemContainerForDownload(form.getEntityId(), getUser(), ReadPermission.class); + if (dataContainer == null) throw new NotFoundException("List does not have an item for the entityid"); - AttachmentParent parent = new ListItemAttachmentParent(form.getEntityId(), getContainer()); + AttachmentParent parent = new ListItemAttachmentParent(form.getEntityId(), dataContainer); return new Pair<>(parent, form.getName()); } diff --git a/list/src/org/labkey/list/model/ListDefinitionImpl.java b/list/src/org/labkey/list/model/ListDefinitionImpl.java index b63b108c2e9..d70f9652601 100644 --- a/list/src/org/labkey/list/model/ListDefinitionImpl.java +++ b/list/src/org/labkey/list/model/ListDefinitionImpl.java @@ -54,6 +54,7 @@ import org.labkey.api.reader.DataLoader; import org.labkey.api.reader.MapLoader; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.Permission; import org.labkey.api.util.ReentrantLockWithName; import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; @@ -521,19 +522,38 @@ private ListItem getListItem(SimpleFilter filter, User user, Container c) return impl; } - public boolean hasListItemForEntityId(String entityId, User user) + public @Nullable Container getListItemContainerForDownload(String entityId, User user, Class permissionClass) { - return hasListItem(new SimpleFilter(FieldKey.fromParts("EntityId"), entityId), user, getContainer()); - } - - private boolean hasListItem(SimpleFilter filter, User user, Container c) - { - TableInfo tbl = getTable(user, c); + Container c = getContainer(); + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("EntityId"), entityId); + // Use a relax CF to find the list items, permission will be validated later + ContainerFilter cf = ContainerFilter.Type.AllInProjectPlusShared.create(c, user); + TableInfo tbl = getTable(user, c, cf); if (null == tbl) - return false; + return null; + + Map row = null; + + try + { + row = new TableSelector(tbl, filter, null).getMap(); + } + catch (IllegalStateException e) + { + // More than one row matches the specified EntityId; log for diagnosis and return null as before + LOG.warn("Multiple list items match EntityId '{}' when resolving download container. List: '{}', Container: '{}'. Returning null.", + entityId, getName(), getContainer().getPath(), e); + } + + if (row == null) + return null; + + Container dataContainer = row.get("Container") != null ? ContainerManager.getForId(row.get("Container").toString()) : null; + if (dataContainer != null && dataContainer.hasPermission(user, permissionClass)) + return dataContainer; - return new TableSelector(tbl, filter, null).exists(); + return null; } @Override diff --git a/list/src/org/labkey/list/model/ListManagerSchema.java b/list/src/org/labkey/list/model/ListManagerSchema.java index ff16718f3d2..7b1854c022c 100644 --- a/list/src/org/labkey/list/model/ListManagerSchema.java +++ b/list/src/org/labkey/list/model/ListManagerSchema.java @@ -21,6 +21,8 @@ import org.labkey.api.data.ActionButton; import org.labkey.api.data.ButtonBar; import org.labkey.api.data.Container; +import org.labkey.api.data.DataRegion; +import org.labkey.api.data.MenuButton; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DisplayColumn; @@ -38,6 +40,7 @@ import org.labkey.api.query.QueryView; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.util.ButtonBuilder; import org.labkey.api.util.LinkBuilder; import org.labkey.api.view.ActionURL; @@ -162,14 +165,37 @@ private ButtonBuilder.Button createImportListArchiveButton() @Override public ActionButton createDeleteButton() { + if (!getContainer().hasPermission(getUser(), DesignListPermission.class)) + return null; + + if (!getContainer().hasPermission(getUser(), AdminPermission.class)) + { + ActionURL urlDelete = new ActionURL(ListController.DeleteListDefinitionAction.class, getContainer()); + urlDelete.addReturnUrl(getReturnUrl()); + ActionButton btnDelete = new ActionButton(urlDelete, "Delete"); + btnDelete.setIconCls("trash"); + btnDelete.setActionType(ActionButton.Action.GET); + btnDelete.setRequiresSelection(true); + return btnDelete; + } + + MenuButton menuDelete = new MenuButton("Delete"); + menuDelete.setIconCls("trash"); + menuDelete.setDisplayPermission(DesignListPermission.class); + menuDelete.setRequiresSelection(true); + ActionURL urlDelete = new ActionURL(ListController.DeleteListDefinitionAction.class, getContainer()); urlDelete.addReturnUrl(getReturnUrl()); - ActionButton btnDelete = new ActionButton(urlDelete, "Delete"); - btnDelete.setIconCls("trash"); - btnDelete.setActionType(ActionButton.Action.GET); - btnDelete.setDisplayPermission(DesignListPermission.class); - btnDelete.setRequiresSelection(true); - return btnDelete; + menuDelete.addMenuItem("Delete List", "if (verifySelected(" + DataRegion.getJavaScriptObjectReference(getDataRegionName()) + ".form, \"" + + urlDelete.getLocalURIString() + "\", \"GET\", \"rows\")) " + DataRegion.getJavaScriptObjectReference(getDataRegionName()) + ".form.submit()"); + + + ActionURL urlTruncate = new ActionURL(ListController.TruncateListDataAction.class, getContainer()); + urlTruncate.addReturnUrl(getReturnUrl()); + menuDelete.addMenuItem("Delete All Data from List", "if (verifySelected(" + DataRegion.getJavaScriptObjectReference(getDataRegionName()) + ".form, \"" + + urlTruncate.getLocalURIString() + "\", \"GET\", \"rows\")) " + DataRegion.getJavaScriptObjectReference(getDataRegionName()) + ".form.submit()"); + + return menuDelete; } private ActionButton createExportArchiveButton() diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index 8110e0c2b13..936cf53af26 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -767,7 +767,7 @@ public void deleteRelatedListData(final User user, final Container container) ListManager.get().deleteIndexedList(_list); // Delete attachments and discussions associated with a list in batches of 1,000 - new TableSelector(getDbTable(), Collections.singleton("entityId")).forEachBatch(String.class, 1000, new ForEachBatchBlock<>() + new TableSelector(getDbTable(), Collections.singleton("entityId"), SimpleFilter.createContainerFilter(container), null).forEachBatch(String.class, 1000, new ForEachBatchBlock<>() { @Override public boolean accept(String entityId) diff --git a/list/src/org/labkey/list/view/ListQueryView.java b/list/src/org/labkey/list/view/ListQueryView.java index a8a79d81e3e..5e01aaa726d 100644 --- a/list/src/org/labkey/list/view/ListQueryView.java +++ b/list/src/org/labkey/list/view/ListQueryView.java @@ -78,8 +78,6 @@ protected void populateButtonBar(DataView view, ButtonBar bar) ActionButton btnUpload = new ActionButton("Design", designURL); bar.add(btnUpload); } - if (canDelete()) - bar.add(super.createDeleteAllRowsButton("list")); } public ListDefinition getList() diff --git a/list/src/org/labkey/list/view/truncateListData.jsp b/list/src/org/labkey/list/view/truncateListData.jsp new file mode 100644 index 00000000000..fae430c126f --- /dev/null +++ b/list/src/org/labkey/list/view/truncateListData.jsp @@ -0,0 +1,70 @@ +<% +/* + * Copyright (c) 2009-2016 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +%> +<%@ page import="org.labkey.api.data.Container" %> +<%@ page import="org.labkey.api.data.TableInfo" %> +<%@ page import="org.labkey.api.data.TableSelector" %> +<%@ page import="org.labkey.api.exp.list.ListDefinition" %> +<%@ page import="org.labkey.api.exp.list.ListService" %> +<%@ page import="org.labkey.list.controllers.ListController" %> +<%@ page import="java.util.ArrayList" %> +<%@ page import="java.util.HashMap" %> +<%@ page import="java.util.List" %> +<%@ page import="java.util.Map" %> +<%@ page import="java.util.Objects" %> +<%@ page import="org.labkey.list.model.ListQuerySchema" %> +<%@ page import="org.labkey.api.security.User" %> +<%@ page extends="org.labkey.api.jsp.FormPage" %> +<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib"%> +<% + ListController.ListDeletionForm form = (ListController.ListDeletionForm)getModelBean(); + Map> definitions = new HashMap<>(); + Container currentContainer = form.getContainer(); + String currentPath = currentContainer.getPath(); + User user = form.getUser(); + ListQuerySchema listQuerySchema = new ListQuerySchema(user, currentContainer); + + form.getListContainerMap() + .stream() + .map(pair -> ListService.get().getList(pair.second, pair.first)) + .filter(Objects::nonNull) + .forEach(listDef -> { + var container = listDef.getContainer(); + if (!definitions.containsKey(container)) + definitions.put(container, new ArrayList<>()); + definitions.get(container).add(listDef); + }); +%> + +

Are you sure you want to delete all data in <%= h(currentPath) %> from the following Lists? This action cannot be undone and will result in an empty list.

+ +<% for (var entry : definitions.entrySet()) { %> +
+ List definitions defined in <%= h(entry.getKey().getPath()) %>: +
    + <% for (var listDef : entry.getValue()) { + TableInfo table = listQuerySchema.getTable(listDef.getName()); + long count = (table != null) ? new TableSelector(table).getRowCount() : 0; + %> +
  • + <%= simpleLink(listDef.getName(), listDef.urlFor(ListController.GridAction.class, currentContainer)) %> + — <%= count %> row<%= h(count != 1 ? "s" : "") %> in <%= h(currentPath) %> +
    • + <% } %> +
+
+<% } %> diff --git a/pipeline/gradle.properties b/pipeline/gradle.properties index 40a5496f023..7960fca9127 100644 --- a/pipeline/gradle.properties +++ b/pipeline/gradle.properties @@ -1,4 +1,4 @@ -activemqVersion=5.18.7 +activemqVersion=5.19.2 geronimoJ2eeConnector15SpecVersion=1.0.1 diff --git a/query/src/org/labkey/query/QueryModule.java b/query/src/org/labkey/query/QueryModule.java index 255be49cbf0..0630786a1a2 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -31,6 +31,7 @@ import org.labkey.api.data.TableInfo; import org.labkey.api.data.views.DataViewService; import org.labkey.api.exp.property.PropertyService; +import org.labkey.api.mcp.McpService; import org.labkey.api.message.digest.DailyMessageDigest; import org.labkey.api.message.digest.ReportAndDatasetChangeDigestProvider; import org.labkey.api.migration.DatabaseMigrationService; @@ -40,7 +41,6 @@ import org.labkey.api.module.DefaultModule; import org.labkey.api.module.Module; import org.labkey.api.module.ModuleContext; -import org.labkey.api.mcp.McpService; import org.labkey.api.pipeline.PipelineService; import org.labkey.api.query.DefaultSchema; import org.labkey.api.query.JavaExportScriptFactory; @@ -77,6 +77,7 @@ import org.labkey.api.security.roles.PlatformDeveloperRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; +import org.labkey.api.settings.OptionalFeatureFlag; import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.stats.AnalyticsProviderRegistry; import org.labkey.api.stats.SummaryStatisticRegistry; @@ -145,6 +146,7 @@ import java.util.function.Supplier; import static org.labkey.api.query.QueryService.USE_ROW_BY_ROW_UPDATE; +import static org.labkey.api.reports.ReportService.R_REPORT_CUSTOM_SHARING; public class QueryModule extends DefaultModule { @@ -346,6 +348,13 @@ public void doStartup(ModuleContext moduleContext) Role trustedAnalystRole = RoleManager.getRole("org.labkey.api.security.roles.TrustedAnalystRole"); if (null != trustedAnalystRole) trustedAnalystRole.addPermission(EditQueriesPermission.class); + + OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(R_REPORT_CUSTOM_SHARING, + "Restore custom R report sharing", + "Allows R reports to be shared on a per user basis. This option will be removed in LabKey Server 26.7.", + false, + false, + OptionalFeatureService.FeatureType.Deprecated)); } @Override diff --git a/study/test/src/org/labkey/test/tests/study/TruncationTest.java b/study/test/src/org/labkey/test/tests/study/TruncationTest.java index 988f3be2b10..917b8e32e83 100644 --- a/study/test/src/org/labkey/test/tests/study/TruncationTest.java +++ b/study/test/src/org/labkey/test/tests/study/TruncationTest.java @@ -79,12 +79,17 @@ private void initTest() public void testTruncateList() { goToProjectHome(); - clickAndWait(Locator.linkWithText(LIST_NAME)); - click(Locator.linkContainingText("Delete All Rows")); - waitAndClick(Ext4Helper.Locators.ext4Button("Yes")); - waitForText("2 rows deleted"); - waitAndClickAndWait(Ext4Helper.Locators.ext4Button("OK")); - waitForText("No data to show."); + var listsPage = goToManageLists(); + var grid = listsPage.getGrid(); + grid.uncheckAllOnPage(); + grid.selectLists(List.of(LIST_NAME)); + grid.clickHeaderMenu("Delete", true, "Delete All Data from List"); + + // Verify confirmation page + assertTextPresent("Are you sure you want to delete all data"); + assertElementPresent(Locator.linkWithText(LIST_NAME)); + assertTextPresent("2 rows"); + clickButton("Confirm Delete All Data"); } @Test