ACL Plugin 7.0.x fixes (#1154)

* fix: add dirty check to acl identity

* fix: do not fetch all records from db

* fix: sid matching, maxOrder, common save

* fix: review comments

* fix: test cases

* fix: remove return

* fix: move method to AclService & test cases

* fix: remove redundant condition

Author: eugene-kamenev

plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclObjectIdentityGormService.groovy

@@ -21,7 +21,6 @@ package grails.plugin.springsecurity.acl
 
 import grails.gorm.DetachedCriteria
 import grails.gorm.transactions.ReadOnly
-import groovy.transform.CompileDynamic
 import groovy.transform.CompileStatic
 import org.springframework.security.acls.model.ObjectIdentity
 
@@ -42,14 +41,9 @@ class AclObjectIdentityGormService {
         AclObjectIdentity.get(id)
     }
 
-    @CompileDynamic
     @ReadOnly
     List<AclObjectIdentity> findAllByParentObjectIdAndParentAclClassName(Long objectId, String aclClassName) {
-        //findQueryByParentObjectIdAndParentAclClassName(objectId, aclClassName).list()
-        List<AclObjectIdentity> aclObjectIdentityList = findAll()
-        aclObjectIdentityList.findAll { AclObjectIdentity oid ->
-            (oid?.parent?.aclClass?.className == aclClassName) &&  ( oid?.parent?.objectId == objectId)
-        }
+        findQueryByParentObjectIdAndParentAclClassName(objectId, aclClassName).list()
     }
 
     @ReadOnly

plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclService.groovy

@@ -22,6 +22,7 @@ import grails.gorm.transactions.ReadOnly
 import groovy.transform.CompileDynamic
 import groovy.transform.CompileStatic
 import groovy.util.logging.Slf4j
+import org.grails.datastore.gorm.GormEntity
 import org.springframework.context.MessageSource
 import org.springframework.security.acls.domain.AccessControlEntryImpl
 import org.springframework.security.acls.domain.GrantedAuthoritySid
@@ -97,10 +98,7 @@ class AclService implements MutableAclService, WarnErros {
 				objectId: object.identifier as Long,
 				owner: ownerSid,
 				entriesInheriting: true)
-		if ( !aclObjectIdentity.save() ) {
-			log.error '{}', errorsBeanBeingSaved(messageSource, aclObjectIdentity)
-		}
-		aclObjectIdentity
+		save(aclObjectIdentity)
 	}
 
 	@Transactional
@@ -124,9 +122,7 @@ class AclService implements MutableAclService, WarnErros {
 		AclSid aclSid = aclSidGormService.findBySidAndPrincipal(sidName, principal)
 		if (!aclSid && allowCreate) {
 			aclSid = new AclSid(sid: sidName, principal: principal)
-			if ( !aclSid.save() ) {
-				log.error '{}', errorsBeanBeingSaved(messageSource, aclSid)
-			}
+			save(aclSid)
 		}
 		aclSid
 	}
@@ -136,9 +132,7 @@ class AclService implements MutableAclService, WarnErros {
 		AclClass aclClass = aclClassGormService.findByClassName(className)
 		if (!aclClass && allowCreate) {
 			aclClass = new AclClass(className: className)
-			if ( !aclClass.save() ) {
-				log.error '{}', errorsBeanBeingSaved(messageSource, aclClass)
-			}
+			save(aclClass)
 		}
 		aclClass
 	}
@@ -209,23 +203,25 @@ class AclService implements MutableAclService, WarnErros {
 			log.trace 'Checking ace for delete: {}', ace
 			!acl.entries.find { AccessControlEntry entry ->
 				log.trace 'Checking entry for delete: {}', entry
-				entry.permission.mask == ace.mask && entry.sid == ace.sid.sid
+				entryEqual(ace, entry)
 			}
 		}
 
 		List<AccessControlEntry> toCreate = acl.entries.findAll { AccessControlEntry entry ->
 			log.trace 'Checking entry for create: {}', entry
 			!existingAces.find { AclEntry ace ->
 				log.trace 'Checking ace for create: {}', ace
-				entry.permission.mask == ace.mask && entry.sid == ace.sid.sid
+				entryEqual(ace, entry)
 			}
 		}
 
+		Integer maxAceOrder = existingAces.max { it.aceOrder }?.aceOrder
+
 		// Delete this ACL's ACEs in the acl_entry table
 		deleteEntries toDelete
 
 		// Create this ACL's ACEs in the acl_entry table
-		createEntries(acl, toCreate as List<AuditableAccessControlEntry>)
+		createEntries(acl, maxAceOrder, toCreate as List<AuditableAccessControlEntry>)
 
 		// Change the mutable columns in acl_object_identity
 		updateObjectIdentity acl
@@ -237,10 +233,9 @@ class AclService implements MutableAclService, WarnErros {
 	}
 
 	@Transactional
-	protected void createEntries(MutableAcl acl, List<AuditableAccessControlEntry> entries = null) {
-		List<AuditableAccessControlEntry> entryList = entries ?: acl.entries as List<AuditableAccessControlEntry>
-		int i = 0
-		for (AuditableAccessControlEntry entry in entryList) {
+	protected void createEntries(MutableAcl acl, Integer maxOrder, List<AuditableAccessControlEntry> entries) {
+		int i = maxOrder != null ? maxOrder + 1 : 0
+		for (AuditableAccessControlEntry entry in entries) {
 			Assert.isInstanceOf AccessControlEntryImpl, entry, 'Unknown ACE class'
 			AclEntry aclEntryInstance = new AclEntry(
 					aclObjectIdentity: AclObjectIdentity.load(acl.id),
@@ -250,9 +245,7 @@ class AclService implements MutableAclService, WarnErros {
 					granting: entry.isGranting(),
 					auditSuccess: entry.isAuditSuccess(),
 					auditFailure: entry.isAuditFailure())
-			if ( !aclEntryInstance.save() ) {
-				log.error "{}", errorsBeanBeingSaved(messageSource, aclEntryInstance)
-			}
+			save(aclEntryInstance)
 		}
 	}
 
@@ -272,9 +265,7 @@ class AclService implements MutableAclService, WarnErros {
 		aclObjectIdentity.parent = parent
 		aclObjectIdentity.owner = createOrRetrieveSid(acl.owner, true)
 		aclObjectIdentity.entriesInheriting = acl.isEntriesInheriting()
-		if ( !aclObjectIdentity.save() ) {
-			log.error "{}", errorsBeanBeingSaved(messageSource, aclObjectIdentity)
-		}
+		save(aclObjectIdentity)
 		AclObjectIdentity.withSession { it.flush() }
 	}
 
@@ -335,4 +326,17 @@ class AclService implements MutableAclService, WarnErros {
 		}
 		return result
 	}
+
+    @Transactional
+	protected <T extends GormEntity<T>> T save(T bean) {
+		if (!bean.save()) {
+            log.warn errorsBeanBeingSaved(messageSource, bean)
+		}
+		bean
+	}
+
+    private static boolean entryEqual(AclEntry ace, AccessControlEntry entry) {
+        Sid sid = ace.sid.principal ? new PrincipalSid(ace.sid.sid) : new GrantedAuthoritySid(ace.sid.sid)
+        return entry.permission.mask == ace.mask && entry.sid == sid && entry.granting == ace.granting
+    }
 }

plugin-acl/plugin/src/main/groovy/grails/plugin/springsecurity/acl/AbstractAclObjectIdentity.groovy

@@ -18,6 +18,7 @@
  */
 package grails.plugin.springsecurity.acl
 
+import grails.gorm.dirty.checking.DirtyCheck
 import groovy.transform.EqualsAndHashCode
 import groovy.transform.ToString
 
@@ -29,6 +30,7 @@ import groovy.transform.ToString
  */
 @EqualsAndHashCode(includes=['aclClass', 'parent', 'owner', 'entriesInheriting'])
 @ToString(includeNames=true)
+@DirtyCheck
 abstract class AbstractAclObjectIdentity implements Serializable {
 
 	AclClass aclClass

plugin-ui/examples/extended/grails-app/domain/grails/plugin/springsecurity/acl/AclObjectIdentity.groovy

@@ -18,12 +18,14 @@
  */
 package grails.plugin.springsecurity.acl
 
+import grails.gorm.dirty.checking.DirtyCheck
 import groovy.transform.ToString
 
 /**
  * @author <a href='mailto:[email protected]'>Burt Beckwith</a>
  */
 @ToString(excludes='version', includeNames=true)
+@DirtyCheck
 class AclObjectIdentity {
 
 	private static final long serialVersionUID = 1

plugin-ui/examples/extended/src/integration-test/groovy/spec/AclEntrySpec.groovy

@@ -54,9 +54,9 @@ class AclEntrySpec extends AbstractSecuritySpec {
 		aclEntrySearchPage.assertResults(1, 3, 3)
 
 		assertContentContains('60')
-		assertContentContains('398')
-		assertContentContains('399')
-		assertContentContains('400')
+		assertContentContains('62')
+		assertContentContains('194')
+		assertContentContains('195')
 		assertContentContains('user1')
 		assertContentContains('admin')
 		assertContentDoesNotContain('>user2</a>')
@@ -74,7 +74,7 @@ class AclEntrySpec extends AbstractSecuritySpec {
 		then:
 		browser.at(AclEntrySearchPage)
 		aclEntrySearchPage.assertResults(1, 10, 67)
-		['104', '111', '119', '126', '131', '136', '141', '146', '152', '159'].each {
+		['75', '76', '78', '80', '82', '87', '89', '91', '93', '95'].each {
 			assertContentContains it
 		}
 	}