From 23e895f0b1b9d73cc725ea9eb60ac2b0a03abe5c Mon Sep 17 00:00:00 2001 From: Jonas Dittrich Date: Thu, 15 Jul 2021 14:03:05 +0200 Subject: [PATCH] Add ObjectIdentityGenerator customization to JdbcAclService Providing the possibility to change, how ObjectIdentitys are created inside the BasicLookupStrategy,JdbcAclService There was a problem with hard coded object identity creation inside the BasicLookupStrategy and the JdbcAclService. It was overkill to overwrite these classes only for changing this, so introducing an ObjectIdentityGenerator seems the be the better solution here. At default, the standard ObjectIdentityRetrievalStrategyImpl is used, but can be customized due to setters. Closes gh-10079 --- .../acls/jdbc/BasicLookupStrategy.java | 19 +- .../security/acls/jdbc/JdbcAclService.java | 204 +++++++++--------- 2 files changed, 121 insertions(+), 102 deletions(-) diff --git a/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java b/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java index 9d4d099b25..f9a0eb4ef6 100644 --- a/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java +++ b/acl/src/main/java/org/springframework/security/acls/jdbc/BasicLookupStrategy.java @@ -35,6 +35,7 @@ import org.springframework.core.convert.ConversionException; import org.springframework.core.convert.ConversionService; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.jdbc.core.ResultSetExtractor; +import org.springframework.security.acls.domain.*; import org.springframework.security.acls.domain.AccessControlEntryImpl; import org.springframework.security.acls.domain.AclAuthorizationStrategy; import org.springframework.security.acls.domain.AclImpl; @@ -42,7 +43,6 @@ import org.springframework.security.acls.domain.AuditLogger; import org.springframework.security.acls.domain.DefaultPermissionFactory; import org.springframework.security.acls.domain.DefaultPermissionGrantingStrategy; import org.springframework.security.acls.domain.GrantedAuthoritySid; -import org.springframework.security.acls.domain.ObjectIdentityImpl; import org.springframework.security.acls.domain.PermissionFactory; import org.springframework.security.acls.domain.PrincipalSid; import org.springframework.security.acls.model.AccessControlEntry; @@ -51,6 +51,7 @@ import org.springframework.security.acls.model.AclCache; import org.springframework.security.acls.model.MutableAcl; import org.springframework.security.acls.model.NotFoundException; import org.springframework.security.acls.model.ObjectIdentity; +import org.springframework.security.acls.model.ObjectIdentityGenerator; import org.springframework.security.acls.model.Permission; import org.springframework.security.acls.model.PermissionGrantingStrategy; import org.springframework.security.acls.model.Sid; @@ -109,6 +110,8 @@ public class BasicLookupStrategy implements LookupStrategy { private final AclAuthorizationStrategy aclAuthorizationStrategy; + private ObjectIdentityGenerator objectIdentityGenerator; + private PermissionFactory permissionFactory = new DefaultPermissionFactory(); private final AclCache aclCache; @@ -134,6 +137,7 @@ public class BasicLookupStrategy implements LookupStrategy { private AclClassIdUtils aclClassIdUtils; + /** * Constructor accepting mandatory arguments * @param dataSource to access the database @@ -152,8 +156,9 @@ public class BasicLookupStrategy implements LookupStrategy { * @param aclAuthorizationStrategy authorization strategy (required) * @param grantingStrategy the PermissionGrantingStrategy */ - public BasicLookupStrategy(DataSource dataSource, AclCache aclCache, - AclAuthorizationStrategy aclAuthorizationStrategy, PermissionGrantingStrategy grantingStrategy) { + public BasicLookupStrategy(DataSource dataSource, AclCache aclCache, + AclAuthorizationStrategy aclAuthorizationStrategy, PermissionGrantingStrategy grantingStrategy) { + Assert.notNull(dataSource, "DataSource required"); Assert.notNull(aclCache, "AclCache required"); Assert.notNull(aclAuthorizationStrategy, "AclAuthorizationStrategy required"); @@ -162,6 +167,7 @@ public class BasicLookupStrategy implements LookupStrategy { this.aclCache = aclCache; this.aclAuthorizationStrategy = aclAuthorizationStrategy; this.grantingStrategy = grantingStrategy; + this.objectIdentityGenerator = new ObjectIdentityRetrievalStrategyImpl(); this.aclClassIdUtils = new AclClassIdUtils(); this.fieldAces.setAccessible(true); this.fieldAcl.setAccessible(true); @@ -488,6 +494,11 @@ public class BasicLookupStrategy implements LookupStrategy { } } + public void setObjectIdentityGenerator(ObjectIdentityGenerator objectIdentityGenerator) { + Assert.notNull(objectIdentityGenerator,"The provided strategy has to be not null!"); + this.objectIdentityGenerator = objectIdentityGenerator; + } + public final void setConversionService(ConversionService conversionService) { this.aclClassIdUtils = new AclClassIdUtils(conversionService); } @@ -569,7 +580,7 @@ public class BasicLookupStrategy implements LookupStrategy { // target id type, e.g. UUID. Serializable identifier = (Serializable) rs.getObject("object_id_identity"); identifier = BasicLookupStrategy.this.aclClassIdUtils.identifierFrom(identifier, rs); - ObjectIdentity objectIdentity = new ObjectIdentityImpl(rs.getString("class"), identifier); + ObjectIdentity objectIdentity = objectIdentityGenerator.createObjectIdentity(identifier,rs.getString("class")); Acl parentAcl = null; long parentAclId = rs.getLong("parent_object"); diff --git a/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java b/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java index 935466f5d1..f1cbc51609 100644 --- a/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java +++ b/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java @@ -31,11 +31,12 @@ import org.apache.commons.logging.LogFactory; import org.springframework.core.convert.ConversionService; import org.springframework.jdbc.core.JdbcOperations; import org.springframework.jdbc.core.JdbcTemplate; -import org.springframework.security.acls.domain.ObjectIdentityImpl; +import org.springframework.security.acls.domain.ObjectIdentityRetrievalStrategyImpl; import org.springframework.security.acls.model.Acl; import org.springframework.security.acls.model.AclService; import org.springframework.security.acls.model.NotFoundException; import org.springframework.security.acls.model.ObjectIdentity; +import org.springframework.security.acls.model.ObjectIdentityGenerator; import org.springframework.security.acls.model.Sid; import org.springframework.util.Assert; @@ -50,123 +51,130 @@ import org.springframework.util.Assert; */ public class JdbcAclService implements AclService { - protected static final Log log = LogFactory.getLog(JdbcAclService.class); + protected static final Log log = LogFactory.getLog(JdbcAclService.class); - private static final String DEFAULT_SELECT_ACL_CLASS_COLUMNS = "class.class as class"; + private static final String DEFAULT_SELECT_ACL_CLASS_COLUMNS = "class.class as class"; - private static final String DEFAULT_SELECT_ACL_CLASS_COLUMNS_WITH_ID_TYPE = DEFAULT_SELECT_ACL_CLASS_COLUMNS - + ", class.class_id_type as class_id_type"; + private static final String DEFAULT_SELECT_ACL_CLASS_COLUMNS_WITH_ID_TYPE = DEFAULT_SELECT_ACL_CLASS_COLUMNS + + ", class.class_id_type as class_id_type"; - private static final String DEFAULT_SELECT_ACL_WITH_PARENT_SQL = "select obj.object_id_identity as obj_id, " - + DEFAULT_SELECT_ACL_CLASS_COLUMNS - + " from acl_object_identity obj, acl_object_identity parent, acl_class class " - + "where obj.parent_object = parent.id and obj.object_id_class = class.id " - + "and parent.object_id_identity = ? and parent.object_id_class = (" - + "select id FROM acl_class where acl_class.class = ?)"; + private static final String DEFAULT_SELECT_ACL_WITH_PARENT_SQL = "select obj.object_id_identity as obj_id, " + + DEFAULT_SELECT_ACL_CLASS_COLUMNS + + " from acl_object_identity obj, acl_object_identity parent, acl_class class " + + "where obj.parent_object = parent.id and obj.object_id_class = class.id " + + "and parent.object_id_identity = ? and parent.object_id_class = (" + + "select id FROM acl_class where acl_class.class = ?)"; - private static final String DEFAULT_SELECT_ACL_WITH_PARENT_SQL_WITH_CLASS_ID_TYPE = "select obj.object_id_identity as obj_id, " - + DEFAULT_SELECT_ACL_CLASS_COLUMNS_WITH_ID_TYPE - + " from acl_object_identity obj, acl_object_identity parent, acl_class class " - + "where obj.parent_object = parent.id and obj.object_id_class = class.id " - + "and parent.object_id_identity = ? and parent.object_id_class = (" - + "select id FROM acl_class where acl_class.class = ?)"; + private static final String DEFAULT_SELECT_ACL_WITH_PARENT_SQL_WITH_CLASS_ID_TYPE = "select obj.object_id_identity as obj_id, " + + DEFAULT_SELECT_ACL_CLASS_COLUMNS_WITH_ID_TYPE + + " from acl_object_identity obj, acl_object_identity parent, acl_class class " + + "where obj.parent_object = parent.id and obj.object_id_class = class.id " + + "and parent.object_id_identity = ? and parent.object_id_class = (" + + "select id FROM acl_class where acl_class.class = ?)"; - protected final JdbcOperations jdbcOperations; + protected final JdbcOperations jdbcOperations; - private final LookupStrategy lookupStrategy; + private final LookupStrategy lookupStrategy; - private boolean aclClassIdSupported; + private boolean aclClassIdSupported; - private String findChildrenSql = DEFAULT_SELECT_ACL_WITH_PARENT_SQL; + private String findChildrenSql = DEFAULT_SELECT_ACL_WITH_PARENT_SQL; - private AclClassIdUtils aclClassIdUtils; + private AclClassIdUtils aclClassIdUtils; + private ObjectIdentityGenerator objectIdentityGenerator; - public JdbcAclService(DataSource dataSource, LookupStrategy lookupStrategy) { - this(new JdbcTemplate(dataSource), lookupStrategy); - } + public JdbcAclService(DataSource dataSource, LookupStrategy lookupStrategy) { + this(new JdbcTemplate(dataSource), lookupStrategy); + } - public JdbcAclService(JdbcOperations jdbcOperations, LookupStrategy lookupStrategy) { - Assert.notNull(jdbcOperations, "JdbcOperations required"); - Assert.notNull(lookupStrategy, "LookupStrategy required"); - this.jdbcOperations = jdbcOperations; - this.lookupStrategy = lookupStrategy; - this.aclClassIdUtils = new AclClassIdUtils(); - } + public JdbcAclService(JdbcOperations jdbcOperations, LookupStrategy lookupStrategy) { + Assert.notNull(jdbcOperations, "JdbcOperations required"); + Assert.notNull(lookupStrategy, "LookupStrategy required"); + this.jdbcOperations = jdbcOperations; + this.lookupStrategy = lookupStrategy; + this.objectIdentityGenerator = new ObjectIdentityRetrievalStrategyImpl(); + this.aclClassIdUtils = new AclClassIdUtils(); + } - @Override - public List findChildren(ObjectIdentity parentIdentity) { - Object[] args = { parentIdentity.getIdentifier().toString(), parentIdentity.getType() }; - List objects = this.jdbcOperations.query(this.findChildrenSql, args, - (rs, rowNum) -> mapObjectIdentityRow(rs)); - return (!objects.isEmpty()) ? objects : null; - } + @Override + public List findChildren(ObjectIdentity parentIdentity) { + Object[] args = {parentIdentity.getIdentifier().toString(), parentIdentity.getType()}; + List objects = this.jdbcOperations.query(this.findChildrenSql, args, + (rs, rowNum) -> mapObjectIdentityRow(rs)); + return (!objects.isEmpty()) ? objects : null; + } - private ObjectIdentity mapObjectIdentityRow(ResultSet rs) throws SQLException { - String javaType = rs.getString("class"); - Serializable identifier = (Serializable) rs.getObject("obj_id"); - identifier = this.aclClassIdUtils.identifierFrom(identifier, rs); - return new ObjectIdentityImpl(javaType, identifier); - } + private ObjectIdentity mapObjectIdentityRow(ResultSet rs) throws SQLException { + String javaType = rs.getString("class"); + Serializable identifier = (Serializable) rs.getObject("obj_id"); + identifier = this.aclClassIdUtils.identifierFrom(identifier, rs); + return objectIdentityGenerator.createObjectIdentity(identifier, javaType); + } - @Override - public Acl readAclById(ObjectIdentity object, List sids) throws NotFoundException { - Map map = readAclsById(Collections.singletonList(object), sids); - Assert.isTrue(map.containsKey(object), - () -> "There should have been an Acl entry for ObjectIdentity " + object); - return map.get(object); - } + @Override + public Acl readAclById(ObjectIdentity object, List sids) throws NotFoundException { + Map map = readAclsById(Collections.singletonList(object), sids); + Assert.isTrue(map.containsKey(object), + () -> "There should have been an Acl entry for ObjectIdentity " + object); + return map.get(object); + } - @Override - public Acl readAclById(ObjectIdentity object) throws NotFoundException { - return readAclById(object, null); - } + @Override + public Acl readAclById(ObjectIdentity object) throws NotFoundException { + return readAclById(object, null); + } - @Override - public Map readAclsById(List objects) throws NotFoundException { - return readAclsById(objects, null); - } + @Override + public Map readAclsById(List objects) throws NotFoundException { + return readAclsById(objects, null); + } - @Override - public Map readAclsById(List objects, List sids) - throws NotFoundException { - Map result = this.lookupStrategy.readAclsById(objects, sids); - // Check every requested object identity was found (throw NotFoundException if - // needed) - for (ObjectIdentity oid : objects) { - if (!result.containsKey(oid)) { - throw new NotFoundException("Unable to find ACL information for object identity '" + oid + "'"); - } - } - return result; - } + @Override + public Map readAclsById(List objects, List sids) + throws NotFoundException { + Map result = this.lookupStrategy.readAclsById(objects, sids); + // Check every requested object identity was found (throw NotFoundException if + // needed) + for (ObjectIdentity oid : objects) { + if (!result.containsKey(oid)) { + throw new NotFoundException("Unable to find ACL information for object identity '" + oid + "'"); + } + } + return result; + } - /** - * Allows customization of the SQL query used to find child object identities. - * @param findChildrenSql - */ - public void setFindChildrenQuery(String findChildrenSql) { - this.findChildrenSql = findChildrenSql; - } + /** + * Allows customization of the SQL query used to find child object identities. + * + * @param findChildrenSql + */ + public void setFindChildrenQuery(String findChildrenSql) { + this.findChildrenSql = findChildrenSql; + } - public void setAclClassIdSupported(boolean aclClassIdSupported) { - this.aclClassIdSupported = aclClassIdSupported; - if (aclClassIdSupported) { - // Change the default children select if it hasn't been overridden - if (this.findChildrenSql.equals(DEFAULT_SELECT_ACL_WITH_PARENT_SQL)) { - this.findChildrenSql = DEFAULT_SELECT_ACL_WITH_PARENT_SQL_WITH_CLASS_ID_TYPE; - } - else { - log.debug("Find children statement has already been overridden, so not overridding the default"); - } - } - } + public void setAclClassIdSupported(boolean aclClassIdSupported) { + this.aclClassIdSupported = aclClassIdSupported; + if (aclClassIdSupported) { + // Change the default children select if it hasn't been overridden + if (this.findChildrenSql.equals(DEFAULT_SELECT_ACL_WITH_PARENT_SQL)) { + this.findChildrenSql = DEFAULT_SELECT_ACL_WITH_PARENT_SQL_WITH_CLASS_ID_TYPE; + } else { + log.debug("Find children statement has already been overridden, so not overridding the default"); + } + } + } - public void setConversionService(ConversionService conversionService) { - this.aclClassIdUtils = new AclClassIdUtils(conversionService); - } + public void setConversionService(ConversionService conversionService) { + this.aclClassIdUtils = new AclClassIdUtils(conversionService); + } - protected boolean isAclClassIdSupported() { - return this.aclClassIdSupported; - } + public void setObjectIdentityGenerator(ObjectIdentityGenerator objectIdentityGenerator) { + Assert.notNull(objectIdentityGenerator,"The provided strategy has to be not null!"); + this.objectIdentityGenerator = objectIdentityGenerator; + } + + protected boolean isAclClassIdSupported() { + return this.aclClassIdSupported; + } }