Skip to content

GH Issue 1353: Clean up DefaultAuditTypeTable.getContainerFilterClause() #8178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
labkey-adam merged 1 commit into from
Oct 14, 2026
Merged

GH Issue 1353: Clean up DefaultAuditTypeTable.getContainerFilterClause() #8178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 1 addition & 13 deletions api/src/org/labkey/api/audit/query/DefaultAuditTypeTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,9 @@
import org.labkey.api.query.QueryUpdateService;
import org.labkey.api.query.UserSchema;
import org.labkey.api.query.column.BuiltInColumnTypes;
import org.labkey.api.security.SecurityManager;
import org.labkey.api.security.User;
import org.labkey.api.security.UserPrincipal;
import org.labkey.api.security.permissions.Permission;
import org.labkey.api.security.permissions.ReadPermission;
import org.labkey.api.security.roles.CanSeeAuditLogRole;
import org.labkey.api.security.roles.Role;
import org.labkey.api.security.roles.RoleManager;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -119,14 +114,7 @@ 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<Role> roles = SecurityManager.canSeeAuditLog(user) ? RoleManager.roleSet(CanSeeAuditLogRole.class) : Set.of();
return filter.createFilterClause(getSchema(), fieldKey, CanSeeAuditLogPermission.class, roles);
return filter.createFilterClause(getSchema(), fieldKey, CanSeeAuditLogPermission.class);
}

// Subclasses may override this to provide customizations to the column
Expand Down
17 changes: 4 additions & 13 deletions api/src/org/labkey/api/data/ContainerFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,7 @@ public FilterClause createFilterClause(DbSchema schema, FieldKey containerFilter
/** Create a FilterClause that restricts based on the containers that meet the filter and user that meets the permission*/
public FilterClause createFilterClause(DbSchema schema, FieldKey containerFilterColumn, Class<? extends Permission> permission)
{
return new ContainerClause(schema, containerFilterColumn, this, permission, Set.of());
}

/** Create a FilterClause that restricts based on the containers that meet the filter and user that meets the permission*/
@Deprecated // TODO: only one use, which we should refactor away. GH Issue 1353
public FilterClause createFilterClause(DbSchema schema, FieldKey containerFilterColumn, Class<? extends Permission> permission, @NotNull Set<Role> contextualRoles)
{
return new ContainerClause(schema, containerFilterColumn, this, permission, contextualRoles);
return new ContainerClause(schema, containerFilterColumn, this, permission);
}

/** Create an expression for a WHERE clause */
Expand Down Expand Up @@ -1312,20 +1305,18 @@ public static class ContainerClause extends FilterClause
private final FieldKey _fieldKey;
private final ContainerFilter _filter;
private final Class<? extends Permission> _permission;
private final @NotNull Set<Role> _contextualRoles;

public ContainerClause(DbSchema schema, FieldKey fieldKey, ContainerFilter filter)
{
this(schema, fieldKey, filter, null, Set.of());
this(schema, fieldKey, filter, null);
}

public ContainerClause(DbSchema schema, FieldKey fieldKey, ContainerFilter filter, @Nullable Class<? extends Permission> permission, @NotNull Set<Role> contextualRoles)
public ContainerClause(DbSchema schema, FieldKey fieldKey, ContainerFilter filter, @Nullable Class<? extends Permission> permission)
{
_schema = schema;
_fieldKey = fieldKey;
_filter = filter;
_permission = permission != null ? permission : ReadPermission.class;
_contextualRoles = contextualRoles;
}

@Override
Expand All @@ -1345,7 +1336,7 @@ public SQLFragment toSQLFragment(Map<FieldKey, ? extends ColumnInfo> columnMap,
{
if (_filter instanceof ContainerFilterWithPermission filter)
{
return filter.getSQLFragment(_schema, _fieldKey, _permission, _contextualRoles);
return filter.getSQLFragment(_schema, _fieldKey, _permission, Set.of());
}
return _filter.getSQLFragment(_schema, _fieldKey, columnMap);
}
Expand Down
4 changes: 2 additions & 2 deletions api/src/org/labkey/api/security/ElevatedUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import org.labkey.api.audit.permissions.CanSeeAuditLogPermission;
import org.labkey.api.data.Container;
import org.labkey.api.security.permissions.Permission;
import org.labkey.api.security.roles.CanSeeAuditLogRole;
import org.labkey.api.security.roles.CanSeeAuditLogFolderRole;
import org.labkey.api.security.roles.Role;
import org.labkey.api.util.Pair;

Expand Down Expand Up @@ -111,6 +111,6 @@ public static User ensureContextualRoles(Container container, User user, Pair<Cl
*/
public static User ensureCanSeeAuditLogRole(Container container, User user)
{
return ensureContextualRoles(container, user, Pair.of(CanSeeAuditLogPermission.class, CanSeeAuditLogRole.class));
return ensureContextualRoles(container, user, Pair.of(CanSeeAuditLogPermission.class, CanSeeAuditLogFolderRole.class));
}
}
6 changes: 3 additions & 3 deletions api/src/org/labkey/api/security/LimitedUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,9 @@ public void testElevatedUser()
Container root = ContainerManager.getRoot();

testPermissions(ElevatedUser.getElevatedUser(new LimitedUser(user, SubmitterRole.class, null), ReaderRole.class, null), 2, 0, true, true, false, false, false);
testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, new LimitedUser(user)), 1, 1, false, false, false, false, true);
testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, new LimitedUser(user, ReaderRole.class)), 2, 1, true, false, false, false, true);
testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, ElevatedUser.getElevatedUser(new LimitedUser(user, ReaderRole.class), EditorRole.class)), 3, 1, true, true, true, false, true);
testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, new LimitedUser(user)), 1, 0, false, false, false, false, true);
testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, new LimitedUser(user, ReaderRole.class)), 2, 0, true, false, false, false, true);
testPermissions(ElevatedUser.ensureCanSeeAuditLogRole(c, ElevatedUser.getElevatedUser(new LimitedUser(user, ReaderRole.class), EditorRole.class)), 3, 0, true, true, true, false, true);

int groupCount = user.getGroups().size();
int roleCount = (int)user.getAssignedRoles(c).count();
Expand Down
10 changes: 0 additions & 10 deletions api/src/org/labkey/api/security/SecurityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import org.labkey.api.action.MutatingApiAction;
import org.labkey.api.action.SpringActionController;
import org.labkey.api.audit.AuditLogService;
import org.labkey.api.audit.permissions.CanSeeAuditLogPermission;
import org.labkey.api.audit.provider.GroupAuditProvider;
import org.labkey.api.cache.CacheManager;
import org.labkey.api.cache.Throttle;
Expand Down Expand Up @@ -2643,15 +2642,6 @@ public static boolean canSeeUserDetails(Container c, User user)
return c.hasPermission(user, SeeUserDetailsPermission.class) || user.hasRootPermission(SeeUserDetailsPermission.class);
}

public static boolean canSeeAuditLog(User user)
{
//
// Only returns true if the user has the site permission. If the user is an admin, then the permission
// check on the current container filter will return true
//
return user.hasRootPermission(CanSeeAuditLogPermission.class);
}

public static boolean canSeeFilePaths(Container c, User user)
{
return (null!=c && c.hasPermission(user, SeeFilePathsPermission.class)) || user.hasRootPermission(SeeFilePathsPermission.class);
Expand Down
12 changes: 3 additions & 9 deletions audit/src/org/labkey/audit/AuditLogImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,10 @@
import org.labkey.api.query.QueryService;
import org.labkey.api.query.UserSchema;
import org.labkey.api.security.LimitedUser;
import org.labkey.api.security.SecurityManager;
import org.labkey.api.security.User;
import org.labkey.api.security.UserManager;
import org.labkey.api.security.permissions.ReadPermission;
import org.labkey.api.security.roles.CanSeeAuditLogRole;
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.ContextListener;
import org.labkey.api.util.GUID;
import org.labkey.api.util.Pair;
Expand Down Expand Up @@ -278,7 +274,7 @@ public record TransactionRowIds(Set<Long> rowIds, Map<Long, Long> dataTypeRowCou
private static Predicate<AuditTypeEvent> inScope(User user, Container container, @Nullable ContainerFilter containerFilter)
{
ContainerFilter cf = null == containerFilter ? ContainerFilter.current(container, user) : containerFilter;
Collection<GUID> ids = scopeIds(user, container, cf);
Collection<GUID> ids = scopeIds(container, cf);
if (null == ids)
return _ -> true;

Expand All @@ -302,14 +298,12 @@ private static Predicate<AuditTypeEvent> inScope(User user, Container container,
* the ReadPermission that {@link ContainerFilter#getIds()} applies, so a folder the user can read but whose audit
* log they cannot see stays out of scope.
*/
private static @Nullable Collection<GUID> scopeIds(User user, Container container, ContainerFilter cf)
private static @Nullable Collection<GUID> scopeIds(Container container, ContainerFilter cf)
{
if (!(cf instanceof ContainerFilter.ContainerFilterWithPermission cfp))
return cf.getIds();

Set<Role> contextualRoles = SecurityManager.canSeeAuditLog(user) ? RoleManager.roleSet(CanSeeAuditLogRole.class) : Set.of();

return cfp.generateIds(container, CanSeeAuditLogPermission.class, contextualRoles);
return cfp.generateIds(container, CanSeeAuditLogPermission.class, Set.of());
}

public TransactionRowIds getTransactionSampleIds(long transactionAuditId, boolean includeInsertEventOnly, User user, Container container, @Nullable ContainerFilter containerFilter)
Expand Down
Loading