JdbcAclService: fixes PostgreSql issue

When setup the acl tables as specified in the Spring.io documentation
I have faced the following error on a PostgreSql database:
org.postgresql.util.PSQLException: ERROR: operator does not exist:
bigint = character varying.
This is because the acl_object_identity.object_id_identity column is
of type varchar(36) but it is not necessarily accessed with a value
of type String.

- JdbcAclService / JdbcMutableAclService: SQL query must match
  object_id_identity column specification
- JdbcAclService: changed JdbcTemplate to JdbcOperations for testability
- JdbcAclServiceTest: Increased test coverage,
  the integration tests using embedded db relates to this commit
cd8d2079ed

Fixes gh-5508

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcAclService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2004, 2005, 2006, 2017 Acegi Technology Pty Limited
+ * Copyright 2004, 2005, 2006, 2017, 2018 Acegi Technology Pty Limited
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import javax.sql.DataSource;
 import org.apache.commons.logging.Log;
 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.jdbc.core.RowMapper;
 import org.springframework.security.acls.domain.ObjectIdentityImpl;
@@ -67,7 +68,7 @@ public class JdbcAclService implements AclService {
 	// ~ Instance fields
 	// ================================================================================================
 
-	protected final JdbcTemplate jdbcTemplate;
+	protected final JdbcOperations jdbcOperations;
 	private final LookupStrategy lookupStrategy;
 	private boolean aclClassIdSupported;
 	private String findChildrenSql = DEFAULT_SELECT_ACL_WITH_PARENT_SQL;
@@ -77,9 +78,13 @@ public class JdbcAclService implements AclService {
 	// ===================================================================================================
 
 	public JdbcAclService(DataSource dataSource, LookupStrategy lookupStrategy) {
-		Assert.notNull(dataSource, "DataSource required");
+		this(new JdbcTemplate(dataSource), lookupStrategy);
+	}
+
+	public JdbcAclService(JdbcOperations jdbcOperations, LookupStrategy lookupStrategy) {
+		Assert.notNull(jdbcOperations, "JdbcOperations required");
 		Assert.notNull(lookupStrategy, "LookupStrategy required");
-		this.jdbcTemplate = new JdbcTemplate(dataSource);
+		this.jdbcOperations = jdbcOperations;
 		this.lookupStrategy = lookupStrategy;
 		this.aclClassIdUtils = new AclClassIdUtils();
 	}
@@ -88,15 +93,14 @@ public class JdbcAclService implements AclService {
 	// ========================================================================================================
 
 	public List<ObjectIdentity> findChildren(ObjectIdentity parentIdentity) {
-		Object[] args = { parentIdentity.getIdentifier(), parentIdentity.getType() };
+		Object[] args = { parentIdentity.getIdentifier().toString(), parentIdentity.getType() };
-		List<ObjectIdentity> objects = jdbcTemplate.query(findChildrenSql, args,
+		List<ObjectIdentity> objects = jdbcOperations.query(findChildrenSql, args,
 				new RowMapper<ObjectIdentity>() {
 					public ObjectIdentity mapRow(ResultSet rs, int rowNum)
 							throws SQLException {
 						String javaType = rs.getString("class");
 						Serializable identifier = (Serializable) rs.getObject("obj_id");
 						identifier = aclClassIdUtils.identifierFrom(identifier, rs);
-
 						return new ObjectIdentityImpl(javaType, identifier);
 					}
 				});

acl/src/main/java/org/springframework/security/acls/jdbc/JdbcMutableAclService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2004, 2005, 2006, 2017 Acegi Technology Pty Limited
+ * Copyright 2004, 2005, 2006, 2017, 2018 Acegi Technology Pty Limited
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -135,7 +135,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 		if (acl.getEntries().isEmpty()) {
 			return;
 		}
-		jdbcTemplate.batchUpdate(insertEntry, new BatchPreparedStatementSetter() {
+		jdbcOperations.batchUpdate(insertEntry, new BatchPreparedStatementSetter() {
 			public int getBatchSize() {
 				return acl.getEntries().size();
 			}
@@ -170,7 +170,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 	protected void createObjectIdentity(ObjectIdentity object, Sid owner) {
 		Long sidId = createOrRetrieveSidPrimaryKey(owner, true);
 		Long classId = createOrRetrieveClassPrimaryKey(object.getType(), true, object.getIdentifier().getClass());
-		jdbcTemplate.update(insertObjectIdentity, classId, object.getIdentifier(), sidId,
+		jdbcOperations.update(insertObjectIdentity, classId, object.getIdentifier().toString(), sidId,
 				Boolean.TRUE);
 	}
 
@@ -184,7 +184,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 	 * @return the primary key or null if not found
 	 */
 	protected Long createOrRetrieveClassPrimaryKey(String type, boolean allowCreate, Class idType) {
-		List<Long> classIds = jdbcTemplate.queryForList(selectClassPrimaryKey,
+		List<Long> classIds = jdbcOperations.queryForList(selectClassPrimaryKey,
 				new Object[] { type }, Long.class);
 
 		if (!classIds.isEmpty()) {
@@ -193,13 +193,13 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 
 		if (allowCreate) {
 			if (!isAclClassIdSupported()) {
-				jdbcTemplate.update(insertClass, type);
+				jdbcOperations.update(insertClass, type);
 			} else {
-				jdbcTemplate.update(insertClass, type, idType.getCanonicalName());
+				jdbcOperations.update(insertClass, type, idType.getCanonicalName());
 			}
 			Assert.isTrue(TransactionSynchronizationManager.isSynchronizationActive(),
 					"Transaction must be running");
-			return jdbcTemplate.queryForObject(classIdentityQuery, Long.class);
+			return jdbcOperations.queryForObject(classIdentityQuery, Long.class);
 		}
 
 		return null;
@@ -248,7 +248,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 	protected Long createOrRetrieveSidPrimaryKey(String sidName, boolean sidIsPrincipal,
 			boolean allowCreate) {
 
-		List<Long> sidIds = jdbcTemplate.queryForList(selectSidPrimaryKey, new Object[] {
+		List<Long> sidIds = jdbcOperations.queryForList(selectSidPrimaryKey, new Object[] {
 				Boolean.valueOf(sidIsPrincipal), sidName }, Long.class);
 
 		if (!sidIds.isEmpty()) {
@@ -256,10 +256,10 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 		}
 
 		if (allowCreate) {
-			jdbcTemplate.update(insertSid, Boolean.valueOf(sidIsPrincipal), sidName);
+			jdbcOperations.update(insertSid, Boolean.valueOf(sidIsPrincipal), sidName);
 			Assert.isTrue(TransactionSynchronizationManager.isSynchronizationActive(),
 					"Transaction must be running");
-			return jdbcTemplate.queryForObject(sidIdentityQuery, Long.class);
+			return jdbcOperations.queryForObject(sidIdentityQuery, Long.class);
 		}
 
 		return null;
@@ -311,7 +311,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 	 * @param oidPrimaryKey the rows in acl_entry to delete
 	 */
 	protected void deleteEntries(Long oidPrimaryKey) {
-		jdbcTemplate.update(deleteEntryByObjectIdentityForeignKey, oidPrimaryKey);
+		jdbcOperations.update(deleteEntryByObjectIdentityForeignKey, oidPrimaryKey);
 	}
 
 	/**
@@ -325,7 +325,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 	 */
 	protected void deleteObjectIdentity(Long oidPrimaryKey) {
 		// Delete the acl_object_identity row
-		jdbcTemplate.update(deleteObjectIdentityByPrimaryKey, oidPrimaryKey);
+		jdbcOperations.update(deleteObjectIdentityByPrimaryKey, oidPrimaryKey);
 	}
 
 	/**
@@ -339,8 +339,8 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 	 */
 	protected Long retrieveObjectIdentityPrimaryKey(ObjectIdentity oid) {
 		try {
-			return jdbcTemplate.queryForObject(selectObjectIdentityPrimaryKey, Long.class,
+			return jdbcOperations.queryForObject(selectObjectIdentityPrimaryKey, Long.class,
-					oid.getType(), oid.getIdentifier());
+					oid.getType(), oid.getIdentifier().toString());
 		}
 		catch (DataAccessException notFound) {
 			return null;
@@ -409,7 +409,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS
 		Assert.notNull(acl.getOwner(), "Owner is required in this implementation");
 
 		Long ownerSid = createOrRetrieveSidPrimaryKey(acl.getOwner(), true);
-		int count = jdbcTemplate.update(updateObjectIdentity, parentId, ownerSid,
+		int count = jdbcOperations.update(updateObjectIdentity, parentId, ownerSid,
 				Boolean.valueOf(acl.isEntriesInheriting()), acl.getId());
 
 		if (count != 1) {

acl/src/test/java/org/springframework/security/acls/jdbc/JdbcAclServiceTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,21 +15,16 @@
  */
 package org.springframework.security.acls.jdbc;
 
-import static org.mockito.ArgumentMatchers.anyList;
+import org.junit.After;
-import static org.mockito.Mockito.when;
-
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-
-import javax.sql.DataSource;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
+import org.springframework.jdbc.core.JdbcOperations;
+import org.springframework.jdbc.core.RowMapper;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabase;
+import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder;
 import org.springframework.security.acls.domain.ObjectIdentityImpl;
 import org.springframework.security.acls.domain.PrincipalSid;
 import org.springframework.security.acls.model.Acl;
@@ -37,19 +32,54 @@ import org.springframework.security.acls.model.NotFoundException;
 import org.springframework.security.acls.model.ObjectIdentity;
 import org.springframework.security.acls.model.Sid;
 
+import javax.sql.DataSource;
+import java.util.*;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.AdditionalMatchers.aryEq;
+import static org.mockito.ArgumentMatchers.*;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit and Integration tests the ACL JdbcAclService using an
+ * in-memory database.
+ *
+ * @author Nena Raab
+ */
 @RunWith(MockitoJUnitRunner.class)
 public class JdbcAclServiceTests {
+
+	private EmbeddedDatabase embeddedDatabase;
+
 	@Mock
 	private DataSource dataSource;
 
 	@Mock
 	private LookupStrategy lookupStrategy;
 
+	@Mock
+	JdbcOperations jdbcOperations;
+
+	private JdbcAclService aclServiceIntegration;
 	private JdbcAclService aclService;
 
 	@Before
 	public void setUp() {
-		aclService = new JdbcAclService(dataSource, lookupStrategy);
+		aclService = new JdbcAclService(jdbcOperations, lookupStrategy);
+		aclServiceIntegration = new JdbcAclService(embeddedDatabase, lookupStrategy);
+	}
+
+	@Before
+	public void setUpEmbeddedDatabase() {
+		embeddedDatabase = new EmbeddedDatabaseBuilder()//
+				.addScript("createAclSchemaWithAclClassIdType.sql")
+				.addScript("db/sql/test_data_hierarchy.sql")
+				.build();
+	}
+
+	@After
+	public void tearDownEmbeddedDatabase() {
+		embeddedDatabase.shutdown();
 	}
 
 	// SEC-1898
@@ -64,4 +94,106 @@ public class JdbcAclServiceTests {
 
 		aclService.readAclById(objectIdentity, sids);
 	}
+
+	@Test
+	public void findOneChildren() {
+		List<ObjectIdentity> result = new ArrayList<>();
+		result.add(new ObjectIdentityImpl(Object.class, "5577"));
+		Object[] args = {"1", "org.springframework.security.acls.jdbc.JdbcAclServiceTests$MockLongIdDomainObject"};
+		when(
+				jdbcOperations.query(anyString(),
+						aryEq(args), any(RowMapper.class))).thenReturn(result);
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl(MockLongIdDomainObject.class, 1L);
+
+		List<ObjectIdentity> objectIdentities = aclService.findChildren(objectIdentity);
+		assertThat(objectIdentities.size()).isEqualTo(1);
+		assertThat(objectIdentities.get(0).getIdentifier()).isEqualTo("5577");
+	}
+
+	@Test
+	public void findNoChildren() {
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl(MockLongIdDomainObject.class, 1L);
+
+		List<ObjectIdentity> objectIdentities = aclService.findChildren(objectIdentity);
+		assertThat(objectIdentities).isNull();
+	}
+
+	// ~ Some integration tests
+	// ========================================================================================================
+
+	@Test
+	public void findChildrenWithoutIdType() {
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl(MockLongIdDomainObject.class, 4711L);
+
+		List<ObjectIdentity> objectIdentities = aclServiceIntegration.findChildren(objectIdentity);
+		assertThat(objectIdentities.size()).isEqualTo(1);
+		assertThat(objectIdentities.get(0).getType()).isEqualTo(MockUntypedIdDomainObject.class.getName());
+		assertThat(objectIdentities.get(0).getIdentifier()).isEqualTo(5000L);
+	}
+
+	@Test
+	public void findChildrenForUnknownObject() {
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl(Object.class, 33);
+
+		List<ObjectIdentity> objectIdentities = aclServiceIntegration.findChildren(objectIdentity);
+		assertThat(objectIdentities).isNull();
+	}
+
+	@Test
+	public void findChildrenOfIdTypeLong() {
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl("location", "US-PAL");
+
+		List<ObjectIdentity> objectIdentities = aclServiceIntegration.findChildren(objectIdentity);
+		assertThat(objectIdentities.size()).isEqualTo(2);
+		assertThat(objectIdentities.get(0).getType()).isEqualTo(MockLongIdDomainObject.class.getName());
+		assertThat(objectIdentities.get(0).getIdentifier()).isEqualTo(4711L);
+		assertThat(objectIdentities.get(1).getType()).isEqualTo(MockLongIdDomainObject.class.getName());
+		assertThat(objectIdentities.get(1).getIdentifier()).isEqualTo(4712L);
+	}
+
+	@Test
+	public void findChildrenOfIdTypeString() {
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl("location", "US");
+
+		aclServiceIntegration.setAclClassIdSupported(true);
+		List<ObjectIdentity> objectIdentities = aclServiceIntegration.findChildren(objectIdentity);
+		assertThat(objectIdentities.size()).isEqualTo(1);
+		assertThat(objectIdentities.get(0).getType()).isEqualTo("location");
+		assertThat(objectIdentities.get(0).getIdentifier()).isEqualTo("US-PAL");
+	}
+
+	@Test
+	public void findChildrenOfIdTypeUUID() {
+		ObjectIdentity objectIdentity = new ObjectIdentityImpl(MockUntypedIdDomainObject.class, 5000L);
+
+		aclServiceIntegration.setAclClassIdSupported(true);
+		List<ObjectIdentity> objectIdentities = aclServiceIntegration.findChildren(objectIdentity);
+		assertThat(objectIdentities.size()).isEqualTo(1);
+		assertThat(objectIdentities.get(0).getType()).isEqualTo("costcenter");
+		assertThat(objectIdentities.get(0).getIdentifier()).isEqualTo(UUID.fromString("25d93b3f-c3aa-4814-9d5e-c7c96ced7762"));
+	}
+
+	private class MockLongIdDomainObject {
+		private Object id;
+
+		public Object getId() {
+			return id;
+		}
+
+		public void setId(Object id) {
+			this.id = id;
+		}
+	}
+
+	private class MockUntypedIdDomainObject {
+		private Object id;
+
+		public Object getId() {
+			return id;
+		}
+
+		public void setId(Object id) {
+			this.id = id;
+		}
+	}
 }

acl/src/test/resources/db/sql/test_data_hierarchy.sql

@@ -0,0 +1,17 @@
+--- insert ACL data
+INSERT INTO ACL_SID (ID, PRINCIPAL, SID) VALUES
+    (10, true, 'user');
+
+INSERT INTO acl_class (id, class, class_id_type) VALUES
+    (20,'location','java.lang.String'),
+    (21,'org.springframework.security.acls.jdbc.JdbcAclServiceTests$MockLongIdDomainObject','java.lang.Long'),
+    (22,'org.springframework.security.acls.jdbc.JdbcAclServiceTests$MockUntypedIdDomainObject',''),
+    (23,'costcenter','java.util.UUID');
+
+INSERT INTO acl_object_identity (id, object_id_class, object_id_identity, parent_object, owner_sid, entries_inheriting) VALUES
+    (1,20,'US',NULL,10,false),
+    (2,20,'US-PAL',1,10,true),
+    (3,21,'4711',2,10,true),
+    (4,21,'4712',2,10,true),
+	(5,22,'5000',3,10,true),
+    (6,23,'25d93b3f-c3aa-4814-9d5e-c7c96ced7762',5,10,true);