This is an automated email from the ASF dual-hosted git repository.
matrei pushed a commit to branch 7.0.x
in repository https://gitbox.apache.org/repos/asf/grails-spring-security.git
The following commit(s) were added to refs/heads/7.0.x by this push:
new 4011ce69 ACL Plugin 7.0.x fixes (#1154)
4011ce69 is described below
commit 4011ce6993fef0392f8f23292787060365c8bcfb
Author: Eugene Kamenev <eugene.kame...@gmail.com>
AuthorDate: Wed Aug 20 18:26:15 2025 +0500
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
---
.../acl/AclObjectIdentityGormService.groovy | 8 +---
.../plugin/springsecurity/acl/AclService.groovy | 50 ++++++++++++----------
.../acl/AbstractAclObjectIdentity.groovy | 2 +
.../springsecurity/acl/AclObjectIdentity.groovy | 2 +
.../groovy/spec/AclEntrySpec.groovy | 8 ++--
5 files changed, 36 insertions(+), 34 deletions(-)
diff --git
a/plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclObjectIdentityGormService.groovy
b/plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclObjectIdentityGormService.groovy
index c72de8c7..a8a42390 100644
---
a/plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclObjectIdentityGormService.groovy
+++
b/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
diff --git
a/plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclService.groovy
b/plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclService.groovy
index 91961a4c..602fe74b 100644
---
a/plugin-acl/plugin/grails-app/services/grails/plugin/springsecurity/acl/AclService.groovy
+++
b/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,7 +203,7 @@ 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)
}
}
@@ -217,15 +211,17 @@ class AclService implements MutableAclService, WarnErros {
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
+ }
}
diff --git
a/plugin-acl/plugin/src/main/groovy/grails/plugin/springsecurity/acl/AbstractAclObjectIdentity.groovy
b/plugin-acl/plugin/src/main/groovy/grails/plugin/springsecurity/acl/AbstractAclObjectIdentity.groovy
index ec5d1275..bb717fb5 100644
---
a/plugin-acl/plugin/src/main/groovy/grails/plugin/springsecurity/acl/AbstractAclObjectIdentity.groovy
+++
b/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
diff --git
a/plugin-ui/examples/extended/grails-app/domain/grails/plugin/springsecurity/acl/AclObjectIdentity.groovy
b/plugin-ui/examples/extended/grails-app/domain/grails/plugin/springsecurity/acl/AclObjectIdentity.groovy
index 306f43a1..061bc98c 100644
---
a/plugin-ui/examples/extended/grails-app/domain/grails/plugin/springsecurity/acl/AclObjectIdentity.groovy
+++
b/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:b...@burtbeckwith.com'>Burt Beckwith</a>
*/
@ToString(excludes='version', includeNames=true)
+@DirtyCheck
class AclObjectIdentity {
private static final long serialVersionUID = 1
diff --git
a/plugin-ui/examples/extended/src/integration-test/groovy/spec/AclEntrySpec.groovy
b/plugin-ui/examples/extended/src/integration-test/groovy/spec/AclEntrySpec.groovy
index 630874ac..c01313db 100644
---
a/plugin-ui/examples/extended/src/integration-test/groovy/spec/AclEntrySpec.groovy
+++
b/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
}
}
