From fdc66d45d54309091e4046879951ca234468f51f Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 30 Jul 2020 09:14:00 -0700 Subject: [PATCH] Polish spring-security-acl main code Manually polish `spring-security-acl` following the formatting and checkstyle fixes. Issue gh-8945 --- .../security/acls/AclEntryVoter.java | 100 ++++++--------- .../acls/AclPermissionCacheOptimizer.java | 17 +-- .../security/acls/AclPermissionEvaluator.java | 53 +++----- .../afterinvocation/AbstractAclProvider.java | 14 +- ...InvocationCollectionFilteringProvider.java | 35 ++--- .../AclEntryAfterInvocationProvider.java | 5 +- .../acls/afterinvocation/ArrayFilterer.java | 67 ++++------ .../afterinvocation/CollectionFilterer.java | 25 +--- .../acls/domain/AccessControlEntryImpl.java | 29 ++--- .../domain/AclAuthorizationStrategyImpl.java | 33 ++--- .../acls/domain/AclFormattingUtils.java | 8 -- .../security/acls/domain/AclImpl.java | 25 +--- .../security/acls/domain/AuditLogger.java | 1 - .../acls/domain/ConsoleAuditLogger.java | 2 - .../acls/domain/CumulativePermission.java | 3 - .../acls/domain/DefaultPermissionFactory.java | 26 +--- .../DefaultPermissionGrantingStrategy.java | 18 +-- .../acls/domain/EhCacheBasedAclCache.java | 34 +---- .../acls/domain/GrantedAuthoritySid.java | 1 - .../acls/domain/ObjectIdentityImpl.java | 28 ++-- .../acls/domain/PermissionFactory.java | 1 - .../security/acls/domain/PrincipalSid.java | 2 - .../acls/domain/SidRetrievalStrategyImpl.java | 3 - .../acls/domain/SpringCacheBasedAclCache.java | 9 -- .../security/acls/jdbc/AclClassIdUtils.java | 42 +++--- .../acls/jdbc/BasicLookupStrategy.java | 120 +++++++----------- .../security/acls/jdbc/JdbcAclService.java | 25 ++-- .../acls/jdbc/JdbcMutableAclService.java | 34 ++--- .../acls/model/AccessControlEntry.java | 1 - .../security/acls/model/AclCache.java | 1 - .../model/AuditableAccessControlEntry.java | 1 - .../security/acls/model/AuditableAcl.java | 1 - .../ObjectIdentityRetrievalStrategy.java | 1 - 33 files changed, 244 insertions(+), 521 deletions(-) diff --git a/acl/src/main/java/org/springframework/security/acls/AclEntryVoter.java b/acl/src/main/java/org/springframework/security/acls/AclEntryVoter.java index 1da5d07d6a..4e694b0247 100644 --- a/acl/src/main/java/org/springframework/security/acls/AclEntryVoter.java +++ b/acl/src/main/java/org/springframework/security/acls/AclEntryVoter.java @@ -41,6 +41,7 @@ import org.springframework.security.acls.model.Sid; import org.springframework.security.acls.model.SidRetrievalStrategy; import org.springframework.security.core.Authentication; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; /** @@ -100,7 +101,11 @@ public class AclEntryVoter extends AbstractAclVoter { private static final Log logger = LogFactory.getLog(AclEntryVoter.class); - private AclService aclService; + private final AclService aclService; + + private final String processConfigAttribute; + + private final List requirePermission; private ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy = new ObjectIdentityRetrievalStrategyImpl(); @@ -108,18 +113,10 @@ public class AclEntryVoter extends AbstractAclVoter { private String internalMethod; - private String processConfigAttribute; - - private List requirePermission; - public AclEntryVoter(AclService aclService, String processConfigAttribute, Permission[] requirePermission) { Assert.notNull(processConfigAttribute, "A processConfigAttribute is mandatory"); Assert.notNull(aclService, "An AclService is mandatory"); - - if ((requirePermission == null) || (requirePermission.length == 0)) { - throw new IllegalArgumentException("One or more requirePermission entries is mandatory"); - } - + Assert.isTrue(!ObjectUtils.isEmpty(requirePermission), "One or more requirePermission entries is mandatory"); this.aclService = aclService; this.processConfigAttribute = processConfigAttribute; this.requirePermission = Arrays.asList(requirePermission); @@ -164,48 +161,24 @@ public class AclEntryVoter extends AbstractAclVoter { @Override public int vote(Authentication authentication, MethodInvocation object, Collection attributes) { - for (ConfigAttribute attr : attributes) { - - if (!this.supports(attr)) { + if (!supports(attr)) { continue; } + // Need to make an access decision on this invocation // Attempt to locate the domain object instance to process Object domainObject = getDomainObjectInstance(object); // If domain object is null, vote to abstain if (domainObject == null) { - if (logger.isDebugEnabled()) { - logger.debug("Voting to abstain - domainObject is null"); - } - + logger.debug("Voting to abstain - domainObject is null"); return ACCESS_ABSTAIN; } // Evaluate if we are required to use an inner domain object if (StringUtils.hasText(this.internalMethod)) { - try { - Class clazz = domainObject.getClass(); - Method method = clazz.getMethod(this.internalMethod, new Class[0]); - domainObject = method.invoke(domainObject); - } - catch (NoSuchMethodException nsme) { - throw new AuthorizationServiceException("Object of class '" + domainObject.getClass() - + "' does not provide the requested internalMethod: " + this.internalMethod); - } - catch (IllegalAccessException iae) { - logger.debug("IllegalAccessException", iae); - - throw new AuthorizationServiceException( - "Problem invoking internalMethod: " + this.internalMethod + " for object: " + domainObject); - } - catch (InvocationTargetException ite) { - logger.debug("InvocationTargetException", ite); - - throw new AuthorizationServiceException( - "Problem invoking internalMethod: " + this.internalMethod + " for object: " + domainObject); - } + domainObject = invokeInternalMethod(domainObject); } // Obtain the OID applicable to the domain object @@ -220,36 +193,21 @@ public class AclEntryVoter extends AbstractAclVoter { // Lookup only ACLs for SIDs we're interested in acl = this.aclService.readAclById(objectIdentity, sids); } - catch (NotFoundException nfe) { - if (logger.isDebugEnabled()) { - logger.debug("Voting to deny access - no ACLs apply for this principal"); - } - + catch (NotFoundException ex) { + logger.debug("Voting to deny access - no ACLs apply for this principal"); return ACCESS_DENIED; } try { if (acl.isGranted(this.requirePermission, sids, false)) { - if (logger.isDebugEnabled()) { - logger.debug("Voting to grant access"); - } - + logger.debug("Voting to grant access"); return ACCESS_GRANTED; } - else { - if (logger.isDebugEnabled()) { - logger.debug( - "Voting to deny access - ACLs returned, but insufficient permissions for this principal"); - } - - return ACCESS_DENIED; - } + logger.debug("Voting to deny access - ACLs returned, but insufficient permissions for this principal"); + return ACCESS_DENIED; } - catch (NotFoundException nfe) { - if (logger.isDebugEnabled()) { - logger.debug("Voting to deny access - no ACLs apply for this principal"); - } - + catch (NotFoundException ex) { + logger.debug("Voting to deny access - no ACLs apply for this principal"); return ACCESS_DENIED; } } @@ -258,4 +216,26 @@ public class AclEntryVoter extends AbstractAclVoter { return ACCESS_ABSTAIN; } + private Object invokeInternalMethod(Object domainObject) { + try { + Class domainObjectType = domainObject.getClass(); + Method method = domainObjectType.getMethod(this.internalMethod, new Class[0]); + return method.invoke(domainObject); + } + catch (NoSuchMethodException ex) { + throw new AuthorizationServiceException("Object of class '" + domainObject.getClass() + + "' does not provide the requested internalMethod: " + this.internalMethod); + } + catch (IllegalAccessException ex) { + logger.debug("IllegalAccessException", ex); + throw new AuthorizationServiceException( + "Problem invoking internalMethod: " + this.internalMethod + " for object: " + domainObject); + } + catch (InvocationTargetException ex) { + logger.debug("InvocationTargetException", ex); + throw new AuthorizationServiceException( + "Problem invoking internalMethod: " + this.internalMethod + " for object: " + domainObject); + } + } + } diff --git a/acl/src/main/java/org/springframework/security/acls/AclPermissionCacheOptimizer.java b/acl/src/main/java/org/springframework/security/acls/AclPermissionCacheOptimizer.java index 25fb52009a..663dcbf174 100644 --- a/acl/src/main/java/org/springframework/security/acls/AclPermissionCacheOptimizer.java +++ b/acl/src/main/java/org/springframework/security/acls/AclPermissionCacheOptimizer.java @@ -23,6 +23,7 @@ import java.util.List; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.PermissionCacheOptimizer; import org.springframework.security.acls.domain.ObjectIdentityRetrievalStrategyImpl; import org.springframework.security.acls.domain.SidRetrievalStrategyImpl; @@ -58,23 +59,15 @@ public class AclPermissionCacheOptimizer implements PermissionCacheOptimizer { if (objects.isEmpty()) { return; } - List oidsToCache = new ArrayList<>(objects.size()); - for (Object domainObject : objects) { - if (domainObject == null) { - continue; + if (domainObject != null) { + ObjectIdentity oid = this.oidRetrievalStrategy.getObjectIdentity(domainObject); + oidsToCache.add(oid); } - ObjectIdentity oid = this.oidRetrievalStrategy.getObjectIdentity(domainObject); - oidsToCache.add(oid); } - List sids = this.sidRetrievalStrategy.getSids(authentication); - - if (this.logger.isDebugEnabled()) { - this.logger.debug("Eagerly loading Acls for " + oidsToCache.size() + " objects"); - } - + this.logger.debug(LogMessage.of(() -> "Eagerly loading Acls for " + oidsToCache.size() + " objects")); this.aclService.readAclsById(oidsToCache, sids); } diff --git a/acl/src/main/java/org/springframework/security/acls/AclPermissionEvaluator.java b/acl/src/main/java/org/springframework/security/acls/AclPermissionEvaluator.java index 568f272c90..15907a22ca 100644 --- a/acl/src/main/java/org/springframework/security/acls/AclPermissionEvaluator.java +++ b/acl/src/main/java/org/springframework/security/acls/AclPermissionEvaluator.java @@ -24,6 +24,7 @@ import java.util.Locale; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.PermissionEvaluator; import org.springframework.security.acls.domain.DefaultPermissionFactory; import org.springframework.security.acls.domain.ObjectIdentityRetrievalStrategyImpl; @@ -76,9 +77,7 @@ public class AclPermissionEvaluator implements PermissionEvaluator { if (domainObject == null) { return false; } - ObjectIdentity objectIdentity = this.objectIdentityRetrievalStrategy.getObjectIdentity(domainObject); - return checkPermission(authentication, objectIdentity, permission); } @@ -86,7 +85,6 @@ public class AclPermissionEvaluator implements PermissionEvaluator { public boolean hasPermission(Authentication authentication, Serializable targetId, String targetType, Object permission) { ObjectIdentity objectIdentity = this.objectIdentityGenerator.createObjectIdentity(targetId, targetType); - return checkPermission(authentication, objectIdentity, permission); } @@ -94,72 +92,51 @@ public class AclPermissionEvaluator implements PermissionEvaluator { // Obtain the SIDs applicable to the principal List sids = this.sidRetrievalStrategy.getSids(authentication); List requiredPermission = resolvePermission(permission); - - final boolean debug = this.logger.isDebugEnabled(); - - if (debug) { - this.logger.debug("Checking permission '" + permission + "' for object '" + oid + "'"); - } - + this.logger.debug(LogMessage.of(() -> "Checking permission '" + permission + "' for object '" + oid + "'")); try { // Lookup only ACLs for SIDs we're interested in Acl acl = this.aclService.readAclById(oid, sids); - if (acl.isGranted(requiredPermission, sids, false)) { - if (debug) { - this.logger.debug("Access is granted"); - } - + this.logger.debug("Access is granted"); return true; } - - if (debug) { - this.logger.debug("Returning false - ACLs returned, but insufficient permissions for this principal"); - } - + this.logger.debug("Returning false - ACLs returned, but insufficient permissions for this principal"); } catch (NotFoundException nfe) { - if (debug) { - this.logger.debug("Returning false - no ACLs apply for this principal"); - } + this.logger.debug("Returning false - no ACLs apply for this principal"); } - return false; - } List resolvePermission(Object permission) { if (permission instanceof Integer) { return Arrays.asList(this.permissionFactory.buildFromMask((Integer) permission)); } - if (permission instanceof Permission) { return Arrays.asList((Permission) permission); } - if (permission instanceof Permission[]) { return Arrays.asList((Permission[]) permission); } - if (permission instanceof String) { String permString = (String) permission; - Permission p; - - try { - p = this.permissionFactory.buildFromName(permString); - } - catch (IllegalArgumentException notfound) { - p = this.permissionFactory.buildFromName(permString.toUpperCase(Locale.ENGLISH)); - } - + Permission p = buildPermission(permString); if (p != null) { return Arrays.asList(p); } - } throw new IllegalArgumentException("Unsupported permission: " + permission); } + private Permission buildPermission(String permString) { + try { + return this.permissionFactory.buildFromName(permString); + } + catch (IllegalArgumentException notfound) { + return this.permissionFactory.buildFromName(permString.toUpperCase(Locale.ENGLISH)); + } + } + public void setObjectIdentityRetrievalStrategy(ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy) { this.objectIdentityRetrievalStrategy = objectIdentityRetrievalStrategy; } diff --git a/acl/src/main/java/org/springframework/security/acls/afterinvocation/AbstractAclProvider.java b/acl/src/main/java/org/springframework/security/acls/afterinvocation/AbstractAclProvider.java index 9f1f5cd9b4..14fb8730d7 100644 --- a/acl/src/main/java/org/springframework/security/acls/afterinvocation/AbstractAclProvider.java +++ b/acl/src/main/java/org/springframework/security/acls/afterinvocation/AbstractAclProvider.java @@ -32,6 +32,7 @@ import org.springframework.security.acls.model.Sid; import org.springframework.security.acls.model.SidRetrievalStrategy; import org.springframework.security.core.Authentication; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; /** * Abstract {@link AfterInvocationProvider} which provides commonly-used ACL-related @@ -43,25 +44,21 @@ public abstract class AbstractAclProvider implements AfterInvocationProvider { protected final AclService aclService; + protected String processConfigAttribute; + protected Class processDomainObjectClass = Object.class; protected ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy = new ObjectIdentityRetrievalStrategyImpl(); protected SidRetrievalStrategy sidRetrievalStrategy = new SidRetrievalStrategyImpl(); - protected String processConfigAttribute; - protected final List requirePermission; public AbstractAclProvider(AclService aclService, String processConfigAttribute, List requirePermission) { Assert.hasText(processConfigAttribute, "A processConfigAttribute is mandatory"); Assert.notNull(aclService, "An AclService is mandatory"); - - if (requirePermission == null || requirePermission.isEmpty()) { - throw new IllegalArgumentException("One or more requirePermission entries is mandatory"); - } - + Assert.isTrue(!ObjectUtils.isEmpty(requirePermission), "One or more requirePermission entries is mandatory"); this.aclService = aclService; this.processConfigAttribute = processConfigAttribute; this.requirePermission = requirePermission; @@ -81,10 +78,9 @@ public abstract class AbstractAclProvider implements AfterInvocationProvider { try { // Lookup only ACLs for SIDs we're interested in Acl acl = this.aclService.readAclById(objectIdentity, sids); - return acl.isGranted(this.requirePermission, sids, false); } - catch (NotFoundException ignore) { + catch (NotFoundException ex) { return false; } } diff --git a/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationCollectionFilteringProvider.java b/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationCollectionFilteringProvider.java index 80d8e5b2c8..fb788322dc 100644 --- a/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationCollectionFilteringProvider.java +++ b/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationCollectionFilteringProvider.java @@ -22,6 +22,7 @@ import java.util.List; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.AuthorizationServiceException; import org.springframework.security.access.ConfigAttribute; @@ -75,10 +76,8 @@ public class AclEntryAfterInvocationCollectionFilteringProvider extends Abstract @SuppressWarnings("unchecked") public Object decide(Authentication authentication, Object object, Collection config, Object returnedObject) throws AccessDeniedException { - if (returnedObject == null) { logger.debug("Return object is null, skipping"); - return null; } @@ -88,18 +87,7 @@ public class AclEntryAfterInvocationCollectionFilteringProvider extends Abstract } // Need to process the Collection for this invocation - Filterer filterer; - - if (returnedObject instanceof Collection) { - filterer = new CollectionFilterer((Collection) returnedObject); - } - else if (returnedObject.getClass().isArray()) { - filterer = new ArrayFilterer((Object[]) returnedObject); - } - else { - throw new AuthorizationServiceException("A Collection or an array (or null) was required as the " - + "returnedObject, but the returnedObject was: " + returnedObject); - } + Filterer filterer = getFilterer(returnedObject); // Locate unauthorised Collection elements for (Object domainObject : filterer) { @@ -108,20 +96,25 @@ public class AclEntryAfterInvocationCollectionFilteringProvider extends Abstract if (domainObject == null || !getProcessDomainObjectClass().isAssignableFrom(domainObject.getClass())) { continue; } - if (!hasPermission(authentication, domainObject)) { filterer.remove(domainObject); - - if (logger.isDebugEnabled()) { - logger.debug("Principal is NOT authorised for element: " + domainObject); - } + logger.debug(LogMessage.of(() -> "Principal is NOT authorised for element: " + domainObject)); } } - return filterer.getFilteredObject(); } - return returnedObject; } + private Filterer getFilterer(Object returnedObject) { + if (returnedObject instanceof Collection) { + return new CollectionFilterer((Collection) returnedObject); + } + if (returnedObject.getClass().isArray()) { + return new ArrayFilterer((Object[]) returnedObject); + } + throw new AuthorizationServiceException("A Collection or an array (or null) was required as the " + + "returnedObject, but the returnedObject was: " + returnedObject); + } + } diff --git a/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationProvider.java b/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationProvider.java index d65017220e..7659cee298 100644 --- a/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationProvider.java +++ b/acl/src/main/java/org/springframework/security/acls/afterinvocation/AclEntryAfterInvocationProvider.java @@ -83,13 +83,11 @@ public class AclEntryAfterInvocationProvider extends AbstractAclProvider impleme // AclManager interface contract prohibits nulls // As they have permission to null/nothing, grant access logger.debug("Return object is null, skipping"); - return null; } if (!getProcessDomainObjectClass().isAssignableFrom(returnedObject.getClass())) { logger.debug("Return object is not applicable for this provider, skipping"); - return returnedObject; } @@ -97,14 +95,13 @@ public class AclEntryAfterInvocationProvider extends AbstractAclProvider impleme if (!this.supports(attr)) { continue; } - // Need to make an access decision on this invocation + // Need to make an access decision on this invocation if (hasPermission(authentication, returnedObject)) { return returnedObject; } logger.debug("Denying access"); - throw new AccessDeniedException(this.messages.getMessage("AclEntryAfterInvocationProvider.noPermission", new Object[] { authentication.getName(), returnedObject }, "Authentication {0} has NO permissions to the domain object {1}")); diff --git a/acl/src/main/java/org/springframework/security/acls/afterinvocation/ArrayFilterer.java b/acl/src/main/java/org/springframework/security/acls/afterinvocation/ArrayFilterer.java index 38ee6610a9..c9c9d01fd5 100644 --- a/acl/src/main/java/org/springframework/security/acls/afterinvocation/ArrayFilterer.java +++ b/acl/src/main/java/org/springframework/security/acls/afterinvocation/ArrayFilterer.java @@ -25,6 +25,8 @@ import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; + /** * A filter used to filter arrays. * @@ -41,17 +43,12 @@ class ArrayFilterer implements Filterer { ArrayFilterer(T[] list) { this.list = list; - // Collect the removed objects to a HashSet so that // it is fast to lookup them when a filtered array // is constructed. this.removeList = new HashSet<>(); } - /** - * - * @see org.springframework.security.acls.afterinvocation.Filterer#getFilteredObject() - */ @Override @SuppressWarnings("unchecked") public T[] getFilteredObject() { @@ -59,60 +56,48 @@ class ArrayFilterer implements Filterer { int originalSize = this.list.length; int sizeOfResultingList = originalSize - this.removeList.size(); T[] filtered = (T[]) Array.newInstance(this.list.getClass().getComponentType(), sizeOfResultingList); - for (int i = 0, j = 0; i < this.list.length; i++) { T object = this.list[i]; - if (!this.removeList.contains(object)) { filtered[j] = object; j++; } } + logger.debug(LogMessage.of(() -> "Original array contained " + originalSize + " elements; now contains " + + sizeOfResultingList + " elements")); + return filtered; + } - if (logger.isDebugEnabled()) { - logger.debug("Original array contained " + originalSize + " elements; now contains " + sizeOfResultingList - + " elements"); - } + @Override + public Iterator iterator() { + return new ArrayFiltererIterator(); + } - return filtered; + @Override + public void remove(T object) { + this.removeList.add(object); } /** - * - * @see org.springframework.security.acls.afterinvocation.Filterer#iterator() + * Iterator for {@link ArrayFilterer} elements. */ - @Override - public Iterator iterator() { - return new Iterator() { - private int index = 0; + private class ArrayFiltererIterator implements Iterator { - @Override - public boolean hasNext() { - return this.index < ArrayFilterer.this.list.length; - } + private int index = 0; - @Override - public T next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - return ArrayFilterer.this.list[this.index++]; - } + @Override + public boolean hasNext() { + return this.index < ArrayFilterer.this.list.length; + } - @Override - public void remove() { - throw new UnsupportedOperationException(); + @Override + public T next() { + if (hasNext()) { + return ArrayFilterer.this.list[this.index++]; } - }; - } + throw new NoSuchElementException(); + } - /** - * - * @see org.springframework.security.acls.afterinvocation.Filterer#remove(java.lang.Object) - */ - @Override - public void remove(T object) { - this.removeList.add(object); } } diff --git a/acl/src/main/java/org/springframework/security/acls/afterinvocation/CollectionFilterer.java b/acl/src/main/java/org/springframework/security/acls/afterinvocation/CollectionFilterer.java index 70a6a0e9cf..8322a9c1aa 100644 --- a/acl/src/main/java/org/springframework/security/acls/afterinvocation/CollectionFilterer.java +++ b/acl/src/main/java/org/springframework/security/acls/afterinvocation/CollectionFilterer.java @@ -24,6 +24,8 @@ import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.core.log.LogMessage; + /** * A filter used to filter Collections. * @@ -40,7 +42,6 @@ class CollectionFilterer implements Filterer { CollectionFilterer(Collection collection) { this.collection = collection; - // We create a Set of objects to be removed from the Collection, // as ConcurrentModificationException prevents removal during // iteration, and making a new Collection to be returned is @@ -51,42 +52,24 @@ class CollectionFilterer implements Filterer { this.removeList = new HashSet<>(); } - /** - * - * @see org.springframework.security.acls.afterinvocation.Filterer#getFilteredObject() - */ @Override public Object getFilteredObject() { // Now the Iterator has ended, remove Objects from Collection Iterator removeIter = this.removeList.iterator(); - int originalSize = this.collection.size(); - while (removeIter.hasNext()) { this.collection.remove(removeIter.next()); } - - if (logger.isDebugEnabled()) { - logger.debug("Original collection contained " + originalSize + " elements; now contains " - + this.collection.size() + " elements"); - } - + logger.debug(LogMessage.of(() -> "Original collection contained " + originalSize + " elements; now contains " + + this.collection.size() + " elements")); return this.collection; } - /** - * - * @see org.springframework.security.acls.afterinvocation.Filterer#iterator() - */ @Override public Iterator iterator() { return this.collection.iterator(); } - /** - * - * @see org.springframework.security.acls.afterinvocation.Filterer#remove(java.lang.Object) - */ @Override public void remove(T object) { this.removeList.add(object); diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java index e58d3d5562..dfc87a5f1f 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/AccessControlEntryImpl.java @@ -65,60 +65,54 @@ public class AccessControlEntryImpl implements AccessControlEntry, AuditableAcce if (!(arg0 instanceof AccessControlEntryImpl)) { return false; } - - AccessControlEntryImpl rhs = (AccessControlEntryImpl) arg0; - + AccessControlEntryImpl other = (AccessControlEntryImpl) arg0; if (this.acl == null) { - if (rhs.getAcl() != null) { + if (other.getAcl() != null) { return false; } // Both this.acl and rhs.acl are null and thus equal } else { // this.acl is non-null - if (rhs.getAcl() == null) { + if (other.getAcl() == null) { return false; } // Both this.acl and rhs.acl are non-null, so do a comparison if (this.acl.getObjectIdentity() == null) { - if (rhs.acl.getObjectIdentity() != null) { + if (other.acl.getObjectIdentity() != null) { return false; } // Both this.acl and rhs.acl are null and thus equal } else { // Both this.acl.objectIdentity and rhs.acl.objectIdentity are non-null - if (!this.acl.getObjectIdentity().equals(rhs.getAcl().getObjectIdentity())) { + if (!this.acl.getObjectIdentity().equals(other.getAcl().getObjectIdentity())) { return false; } } } - if (this.id == null) { - if (rhs.id != null) { + if (other.id != null) { return false; } // Both this.id and rhs.id are null and thus equal } else { // this.id is non-null - if (rhs.id == null) { + if (other.id == null) { return false; } - // Both this.id and rhs.id are non-null - if (!this.id.equals(rhs.id)) { + if (!this.id.equals(other.id)) { return false; } } - - if ((this.auditFailure != rhs.isAuditFailure()) || (this.auditSuccess != rhs.isAuditSuccess()) - || (this.granting != rhs.isGranting()) || !this.permission.equals(rhs.getPermission()) - || !this.sid.equals(rhs.getSid())) { + if ((this.auditFailure != other.isAuditFailure()) || (this.auditSuccess != other.isAuditSuccess()) + || (this.granting != other.isGranting()) || !this.permission.equals(other.getPermission()) + || !this.sid.equals(other.getSid())) { return false; } - return true; } @@ -192,7 +186,6 @@ public class AccessControlEntryImpl implements AccessControlEntry, AuditableAcce sb.append("auditSuccess: ").append(this.auditSuccess).append("; "); sb.append("auditFailure: ").append(this.auditFailure); sb.append("]"); - return sb.toString(); } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AclAuthorizationStrategyImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/AclAuthorizationStrategyImpl.java index 92e621bc2d..34e62babb5 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AclAuthorizationStrategyImpl.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/AclAuthorizationStrategyImpl.java @@ -86,32 +86,15 @@ public class AclAuthorizationStrategyImpl implements AclAuthorizationStrategy { || !SecurityContextHolder.getContext().getAuthentication().isAuthenticated()) { throw new AccessDeniedException("Authenticated principal required to operate with ACLs"); } - Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); - // Check if authorized by virtue of ACL ownership Sid currentUser = createCurrentUser(authentication); - if (currentUser.equals(acl.getOwner()) && ((changeType == CHANGE_GENERAL) || (changeType == CHANGE_OWNERSHIP))) { return; } - // Not authorized by ACL ownership; try via adminstrative permissions - GrantedAuthority requiredAuthority; - - if (changeType == CHANGE_AUDITING) { - requiredAuthority = this.gaModifyAuditing; - } - else if (changeType == CHANGE_GENERAL) { - requiredAuthority = this.gaGeneralChanges; - } - else if (changeType == CHANGE_OWNERSHIP) { - requiredAuthority = this.gaTakeOwnership; - } - else { - throw new IllegalArgumentException("Unknown change type"); - } + GrantedAuthority requiredAuthority = getRequiredAuthority(changeType); // Iterate this principal's authorities to determine right Set authorities = AuthorityUtils.authorityListToSet(authentication.getAuthorities()); @@ -121,7 +104,6 @@ public class AclAuthorizationStrategyImpl implements AclAuthorizationStrategy { // Try to get permission via ACEs within the ACL List sids = this.sidRetrievalStrategy.getSids(authentication); - if (acl.isGranted(Arrays.asList(BasePermission.ADMINISTRATION), sids, false)) { return; } @@ -130,6 +112,19 @@ public class AclAuthorizationStrategyImpl implements AclAuthorizationStrategy { "Principal does not have required ACL permissions to perform requested operation"); } + private GrantedAuthority getRequiredAuthority(int changeType) { + if (changeType == CHANGE_AUDITING) { + return this.gaModifyAuditing; + } + if (changeType == CHANGE_GENERAL) { + return this.gaGeneralChanges; + } + if (changeType == CHANGE_OWNERSHIP) { + return this.gaTakeOwnership; + } + throw new IllegalArgumentException("Unknown change type"); + } + /** * Creates a principal-like sid from the authentication information. * @param authentication the authentication information that can provide principal and diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AclFormattingUtils.java b/acl/src/main/java/org/springframework/security/acls/domain/AclFormattingUtils.java index 0922aa38bf..36bdb28a2d 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AclFormattingUtils.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/AclFormattingUtils.java @@ -31,9 +31,7 @@ public abstract class AclFormattingUtils { Assert.notNull(removeBits, "Bits To Remove string required"); Assert.isTrue(original.length() == removeBits.length(), "Original and Bits To Remove strings must be identical length"); - char[] replacement = new char[original.length()]; - for (int i = 0; i < original.length(); i++) { if (removeBits.charAt(i) == Permission.RESERVED_OFF) { replacement[i] = original.charAt(i); @@ -42,7 +40,6 @@ public abstract class AclFormattingUtils { replacement[i] = Permission.RESERVED_OFF; } } - return new String(replacement); } @@ -51,9 +48,7 @@ public abstract class AclFormattingUtils { Assert.notNull(extraBits, "Extra Bits string required"); Assert.isTrue(original.length() == extraBits.length(), "Original and Extra Bits strings must be identical length"); - char[] replacement = new char[extraBits.length()]; - for (int i = 0; i < extraBits.length(); i++) { if (extraBits.charAt(i) == Permission.RESERVED_OFF) { replacement[i] = original.charAt(i); @@ -62,7 +57,6 @@ public abstract class AclFormattingUtils { replacement[i] = extraBits.charAt(i); } } - return new String(replacement); } @@ -92,7 +86,6 @@ public abstract class AclFormattingUtils { () -> Permission.RESERVED_ON + " is a reserved character code"); Assert.doesNotContain(Character.toString(code), Character.toString(Permission.RESERVED_OFF), () -> Permission.RESERVED_OFF + " is a reserved character code"); - return printBinary(mask, Permission.RESERVED_ON, Permission.RESERVED_OFF).replace(Permission.RESERVED_ON, code); } @@ -100,7 +93,6 @@ public abstract class AclFormattingUtils { String s = Integer.toBinaryString(i); String pattern = Permission.THIRTY_TWO_RESERVED_OFF; String temp2 = pattern.substring(0, pattern.length() - s.length()) + s; - return temp2.replace('0', off).replace('1', on); } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java index bbba414e96..5f7a0532a3 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/AclImpl.java @@ -53,10 +53,11 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { private Serializable id; - private Sid owner; // OwnershipAcl + // OwnershipAcl + private Sid owner; - private List loadedSids = null; // includes all SIDs the WHERE clause covered, - // even if there was no ACE for a SID + // includes all SIDs the WHERE clause covered, even if there was no ACE for a SID + private List loadedSids = null; private boolean entriesInheriting = true; @@ -102,7 +103,6 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { Assert.notNull(id, "Id required"); Assert.notNull(aclAuthorizationStrategy, "AclAuthorizationStrategy required"); Assert.notNull(owner, "Owner required"); - this.objectIdentity = objectIdentity; this.id = id; this.aclAuthorizationStrategy = aclAuthorizationStrategy; @@ -125,7 +125,6 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { public void deleteAce(int aceIndex) throws NotFoundException { this.aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL); verifyAceIndexExists(aceIndex); - synchronized (this.aces) { this.aces.remove(aceIndex); } @@ -154,9 +153,7 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { throw new NotFoundException( "atIndexLocation must be less than or equal to the size of the AccessControlEntry collection"); } - AccessControlEntryImpl ace = new AccessControlEntryImpl(null, this, sid, permission, granting, false, false); - synchronized (this.aces) { this.aces.add(atIndexLocation, ace); } @@ -195,11 +192,9 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { throws NotFoundException, UnloadedSidException { Assert.notEmpty(permission, "Permissions required"); Assert.notEmpty(sids, "SIDs required"); - if (!this.isSidLoaded(sids)) { throw new UnloadedSidException("ACL was not loaded for one or more SID"); } - return this.permissionGrantingStrategy.isGranted(this, permission, sids, administrativeMode); } @@ -214,16 +209,13 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { // This ACL applies to a SID subset only. Iterate to check it applies. for (Sid sid : sids) { boolean found = false; - for (Sid loadedSid : this.loadedSids) { if (sid.equals(loadedSid)) { // this SID is OK found = true; - break; // out of loadedSids for loop } } - if (!found) { return false; } @@ -266,7 +258,6 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { public void updateAce(int aceIndex, Permission permission) throws NotFoundException { this.aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_GENERAL); verifyAceIndexExists(aceIndex); - synchronized (this.aces) { AccessControlEntryImpl ace = (AccessControlEntryImpl) this.aces.get(aceIndex); ace.setPermission(permission); @@ -277,7 +268,6 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { public void updateAuditing(int aceIndex, boolean auditSuccess, boolean auditFailure) { this.aclAuthorizationStrategy.securityCheck(this, AclAuthorizationStrategy.CHANGE_AUDITING); verifyAceIndexExists(aceIndex); - synchronized (this.aces) { AccessControlEntryImpl ace = (AccessControlEntryImpl) this.aces.get(aceIndex); ace.setAuditSuccess(auditSuccess); @@ -327,30 +317,23 @@ public class AclImpl implements Acl, MutableAcl, AuditableAcl, OwnershipAcl { sb.append("id: ").append(this.id).append("; "); sb.append("objectIdentity: ").append(this.objectIdentity).append("; "); sb.append("owner: ").append(this.owner).append("; "); - int count = 0; - for (AccessControlEntry ace : this.aces) { count++; - if (count == 1) { sb.append("\n"); } - sb.append(ace).append("\n"); } - if (count == 0) { sb.append("no ACEs; "); } - sb.append("inheriting: ").append(this.entriesInheriting).append("; "); sb.append("parent: ").append((this.parentAcl == null) ? "Null" : this.parentAcl.getObjectIdentity().toString()); sb.append("; "); sb.append("aclAuthorizationStrategy: ").append(this.aclAuthorizationStrategy).append("; "); sb.append("permissionGrantingStrategy: ").append(this.permissionGrantingStrategy); sb.append("]"); - return sb.toString(); } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/AuditLogger.java b/acl/src/main/java/org/springframework/security/acls/domain/AuditLogger.java index 0003f44154..20edb6aa23 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/AuditLogger.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/AuditLogger.java @@ -22,7 +22,6 @@ import org.springframework.security.acls.model.AccessControlEntry; * Used by AclImpl to log audit events. * * @author Ben Alex - * */ public interface AuditLogger { diff --git a/acl/src/main/java/org/springframework/security/acls/domain/ConsoleAuditLogger.java b/acl/src/main/java/org/springframework/security/acls/domain/ConsoleAuditLogger.java index ee2aec26a8..744ec34148 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/ConsoleAuditLogger.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/ConsoleAuditLogger.java @@ -30,10 +30,8 @@ public class ConsoleAuditLogger implements AuditLogger { @Override public void logIfNeeded(boolean granted, AccessControlEntry ace) { Assert.notNull(ace, "AccessControlEntry required"); - if (ace instanceof AuditableAccessControlEntry) { AuditableAccessControlEntry auditableAce = (AuditableAccessControlEntry) ace; - if (granted && auditableAce.isAuditSuccess()) { System.out.println("GRANTED due to ACE: " + ace); } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/CumulativePermission.java b/acl/src/main/java/org/springframework/security/acls/domain/CumulativePermission.java index d895f0bb46..819ce4e5f3 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/CumulativePermission.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/CumulativePermission.java @@ -39,21 +39,18 @@ public class CumulativePermission extends AbstractPermission { public CumulativePermission clear(Permission permission) { this.mask &= ~permission.getMask(); this.pattern = AclFormattingUtils.demergePatterns(this.pattern, permission.getPattern()); - return this; } public CumulativePermission clear() { this.mask = 0; this.pattern = THIRTY_TWO_RESERVED_OFF; - return this; } public CumulativePermission set(Permission permission) { this.mask |= permission.getMask(); this.pattern = AclFormattingUtils.mergePatterns(this.pattern, permission.getPattern()); - return this; } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionFactory.java b/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionFactory.java index abc67f6d01..8a68843d89 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionFactory.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionFactory.java @@ -77,22 +77,18 @@ public class DefaultPermissionFactory implements PermissionFactory { */ protected void registerPublicPermissions(Class clazz) { Assert.notNull(clazz, "Class required"); - Field[] fields = clazz.getFields(); - for (Field field : fields) { try { Object fieldValue = field.get(null); - if (Permission.class.isAssignableFrom(fieldValue.getClass())) { // Found a Permission static field Permission perm = (Permission) fieldValue; String permissionName = field.getName(); - registerPermission(perm, permissionName); } } - catch (Exception ignore) { + catch (Exception ex) { } } } @@ -100,7 +96,6 @@ public class DefaultPermissionFactory implements PermissionFactory { protected void registerPermission(Permission perm, String permissionName) { Assert.notNull(perm, "Permission required"); Assert.hasText(permissionName, "Permission name required"); - Integer mask = perm.getMask(); // Ensure no existing Permission uses this integer or code @@ -124,32 +119,22 @@ public class DefaultPermissionFactory implements PermissionFactory { // To get this far, we have to use a CumulativePermission CumulativePermission permission = new CumulativePermission(); - for (int i = 0; i < 32; i++) { int permissionToCheck = 1 << i; - if ((mask & permissionToCheck) == permissionToCheck) { Permission p = this.registeredPermissionsByInteger.get(permissionToCheck); - - if (p == null) { - throw new IllegalStateException( - "Mask '" + permissionToCheck + "' does not have a corresponding static Permission"); - } + Assert.state(p != null, + () -> "Mask '" + permissionToCheck + "' does not have a corresponding static Permission"); permission.set(p); } } - return permission; } @Override public Permission buildFromName(String name) { Permission p = this.registeredPermissionsByName.get(name); - - if (p == null) { - throw new IllegalArgumentException("Unknown permission '" + name + "'"); - } - + Assert.notNull(p, "Unknown permission '" + name + "'"); return p; } @@ -158,13 +143,10 @@ public class DefaultPermissionFactory implements PermissionFactory { if ((names == null) || (names.size() == 0)) { return Collections.emptyList(); } - List permissions = new ArrayList<>(names.size()); - for (String name : names) { permissions.add(buildFromName(name)); } - return permissions; } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionGrantingStrategy.java b/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionGrantingStrategy.java index 6b3d7ae2fb..2bfc050be2 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionGrantingStrategy.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/DefaultPermissionGrantingStrategy.java @@ -74,18 +74,13 @@ public class DefaultPermissionGrantingStrategy implements PermissionGrantingStra @Override public boolean isGranted(Acl acl, List permission, List sids, boolean administrativeMode) throws NotFoundException { - - final List aces = acl.getEntries(); - + List aces = acl.getEntries(); AccessControlEntry firstRejection = null; - for (Permission p : permission) { for (Sid sid : sids) { // Attempt to find exact match for this permission mask and SID boolean scanNextSid = true; - for (AccessControlEntry ace : aces) { - if (isGranted(ace, p) && ace.getSid().equals(sid)) { // Found a matching ACE, so its authorization decision will // prevail @@ -94,7 +89,6 @@ public class DefaultPermissionGrantingStrategy implements PermissionGrantingStra if (!administrativeMode) { this.auditLogger.logIfNeeded(true, ace); } - return true; } @@ -105,13 +99,11 @@ public class DefaultPermissionGrantingStrategy implements PermissionGrantingStra // Store first rejection for auditing reasons firstRejection = ace; } - scanNextSid = false; // helps break the loop break; // exit aces loop } } - if (!scanNextSid) { break; // exit SID for loop (now try next permission) } @@ -124,7 +116,6 @@ public class DefaultPermissionGrantingStrategy implements PermissionGrantingStra if (!administrativeMode) { this.auditLogger.logIfNeeded(false, firstRejection); } - return false; } @@ -133,10 +124,9 @@ public class DefaultPermissionGrantingStrategy implements PermissionGrantingStra // We have a parent, so let them try to find a matching ACE return acl.getParentAcl().isGranted(permission, sids, false); } - else { - // We either have no parent, or we're the uppermost parent - throw new NotFoundException("Unable to locate a matching ACE for passed permissions and SIDs"); - } + + // We either have no parent, or we're the uppermost parent + throw new NotFoundException("Unable to locate a matching ACE for passed permissions and SIDs"); } /** diff --git a/acl/src/main/java/org/springframework/security/acls/domain/EhCacheBasedAclCache.java b/acl/src/main/java/org/springframework/security/acls/domain/EhCacheBasedAclCache.java index 0a7af4a255..124ec9671d 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/EhCacheBasedAclCache.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/EhCacheBasedAclCache.java @@ -59,9 +59,7 @@ public class EhCacheBasedAclCache implements AclCache { @Override public void evictFromCache(Serializable pk) { Assert.notNull(pk, "Primary key (identifier) required"); - MutableAcl acl = getFromCache(pk); - if (acl != null) { this.cache.remove(acl.getId()); this.cache.remove(acl.getObjectIdentity()); @@ -71,9 +69,7 @@ public class EhCacheBasedAclCache implements AclCache { @Override public void evictFromCache(ObjectIdentity objectIdentity) { Assert.notNull(objectIdentity, "ObjectIdentity required"); - MutableAcl acl = getFromCache(objectIdentity); - if (acl != null) { this.cache.remove(acl.getId()); this.cache.remove(acl.getObjectIdentity()); @@ -83,39 +79,25 @@ public class EhCacheBasedAclCache implements AclCache { @Override public MutableAcl getFromCache(ObjectIdentity objectIdentity) { Assert.notNull(objectIdentity, "ObjectIdentity required"); - - Element element = null; - try { - element = this.cache.get(objectIdentity); - } - catch (CacheException ignored) { + Element element = this.cache.get(objectIdentity); + return (element != null) ? initializeTransientFields((MutableAcl) element.getValue()) : null; } - - if (element == null) { + catch (CacheException ex) { return null; } - - return initializeTransientFields((MutableAcl) element.getValue()); } @Override public MutableAcl getFromCache(Serializable pk) { Assert.notNull(pk, "Primary key (identifier) required"); - - Element element = null; - try { - element = this.cache.get(pk); - } - catch (CacheException ignored) { + Element element = this.cache.get(pk); + return (element != null) ? initializeTransientFields((MutableAcl) element.getValue()) : null; } - - if (element == null) { + catch (CacheException ex) { return null; } - - return initializeTransientFields((MutableAcl) element.getValue()); } @Override @@ -123,7 +105,6 @@ public class EhCacheBasedAclCache implements AclCache { Assert.notNull(acl, "Acl required"); Assert.notNull(acl.getObjectIdentity(), "ObjectIdentity required"); Assert.notNull(acl.getId(), "ID required"); - if (this.aclAuthorizationStrategy == null) { if (acl instanceof AclImpl) { this.aclAuthorizationStrategy = (AclAuthorizationStrategy) FieldUtils @@ -132,11 +113,9 @@ public class EhCacheBasedAclCache implements AclCache { .getProtectedFieldValue("permissionGrantingStrategy", acl); } } - if ((acl.getParentAcl() != null) && (acl.getParentAcl() instanceof MutableAcl)) { putInCache((MutableAcl) acl.getParentAcl()); } - this.cache.put(new Element(acl.getObjectIdentity(), acl)); this.cache.put(new Element(acl.getId(), acl)); } @@ -146,7 +125,6 @@ public class EhCacheBasedAclCache implements AclCache { FieldUtils.setProtectedFieldValue("aclAuthorizationStrategy", value, this.aclAuthorizationStrategy); FieldUtils.setProtectedFieldValue("permissionGrantingStrategy", value, this.permissionGrantingStrategy); } - if (value.getParentAcl() != null) { initializeTransientFields((MutableAcl) value.getParentAcl()); } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/GrantedAuthoritySid.java b/acl/src/main/java/org/springframework/security/acls/domain/GrantedAuthoritySid.java index 64eb57aacd..73c1dc0366 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/GrantedAuthoritySid.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/GrantedAuthoritySid.java @@ -51,7 +51,6 @@ public class GrantedAuthoritySid implements Sid { if ((object == null) || !(object instanceof GrantedAuthoritySid)) { return false; } - // Delegate to getGrantedAuthority() to perform actual comparison (both should be // identical) return ((GrantedAuthoritySid) object).getGrantedAuthority().equals(this.getGrantedAuthority()); diff --git a/acl/src/main/java/org/springframework/security/acls/domain/ObjectIdentityImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/ObjectIdentityImpl.java index 61018e349c..aafa3fff3f 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/ObjectIdentityImpl.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/ObjectIdentityImpl.java @@ -40,7 +40,6 @@ public class ObjectIdentityImpl implements ObjectIdentity { public ObjectIdentityImpl(String type, Serializable identifier) { Assert.hasText(type, "Type required"); Assert.notNull(identifier, "identifier required"); - this.identifier = identifier; this.type = type; } @@ -68,23 +67,22 @@ public class ObjectIdentityImpl implements ObjectIdentity { */ public ObjectIdentityImpl(Object object) throws IdentityUnavailableException { Assert.notNull(object, "object cannot be null"); - Class typeClass = ClassUtils.getUserClass(object.getClass()); this.type = typeClass.getName(); + Object result = invokeGetIdMethod(object, typeClass); + Assert.notNull(result, "getId() is required to return a non-null value"); + Assert.isInstanceOf(Serializable.class, result, "Getter must provide a return value of type Serializable"); + this.identifier = (Serializable) result; + } - Object result; - + private Object invokeGetIdMethod(Object object, Class typeClass) { try { Method method = typeClass.getMethod("getId", new Class[] {}); - result = method.invoke(object); + return method.invoke(object); } catch (Exception ex) { throw new IdentityUnavailableException("Could not extract identity from object " + object, ex); } - - Assert.notNull(result, "getId() is required to return a non-null value"); - Assert.isInstanceOf(Serializable.class, result, "Getter must provide a return value of type Serializable"); - this.identifier = (Serializable) result; } /** @@ -95,17 +93,15 @@ public class ObjectIdentityImpl implements ObjectIdentity { *

* Numeric identities (Integer and Long values) are considered equal if they are * numerically equal. Other serializable types are evaluated using a simple equality. - * @param arg0 object to compare + * @param obj object to compare * @return true if the presented object matches this object */ @Override - public boolean equals(Object arg0) { - if (arg0 == null || !(arg0 instanceof ObjectIdentityImpl)) { + public boolean equals(Object obj) { + if (obj == null || !(obj instanceof ObjectIdentityImpl)) { return false; } - - ObjectIdentityImpl other = (ObjectIdentityImpl) arg0; - + ObjectIdentityImpl other = (ObjectIdentityImpl) obj; if (this.identifier instanceof Number && other.identifier instanceof Number) { // Integers and Longs with same value should be considered equal if (((Number) this.identifier).longValue() != ((Number) other.identifier).longValue()) { @@ -118,7 +114,6 @@ public class ObjectIdentityImpl implements ObjectIdentity { return false; } } - return this.type.equals(other.type); } @@ -149,7 +144,6 @@ public class ObjectIdentityImpl implements ObjectIdentity { sb.append(this.getClass().getName()).append("["); sb.append("Type: ").append(this.type); sb.append("; Identifier: ").append(this.identifier).append("]"); - return sb.toString(); } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/PermissionFactory.java b/acl/src/main/java/org/springframework/security/acls/domain/PermissionFactory.java index 70ca75701e..41b613274e 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/PermissionFactory.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/PermissionFactory.java @@ -26,7 +26,6 @@ import org.springframework.security.acls.model.Permission; * * @author Ben Alex * @since 2.0.3 - * */ public interface PermissionFactory { diff --git a/acl/src/main/java/org/springframework/security/acls/domain/PrincipalSid.java b/acl/src/main/java/org/springframework/security/acls/domain/PrincipalSid.java index 1dc357762f..373d85a5e9 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/PrincipalSid.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/PrincipalSid.java @@ -42,7 +42,6 @@ public class PrincipalSid implements Sid { public PrincipalSid(Authentication authentication) { Assert.notNull(authentication, "Authentication required"); Assert.notNull(authentication.getPrincipal(), "Principal required"); - this.principal = authentication.getName(); } @@ -51,7 +50,6 @@ public class PrincipalSid implements Sid { if ((object == null) || !(object instanceof PrincipalSid)) { return false; } - // Delegate to getPrincipal() to perform actual comparison (both should be // identical) return ((PrincipalSid) object).getPrincipal().equals(this.getPrincipal()); diff --git a/acl/src/main/java/org/springframework/security/acls/domain/SidRetrievalStrategyImpl.java b/acl/src/main/java/org/springframework/security/acls/domain/SidRetrievalStrategyImpl.java index 5a82c3e6ac..92e0e6a224 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/SidRetrievalStrategyImpl.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/SidRetrievalStrategyImpl.java @@ -56,13 +56,10 @@ public class SidRetrievalStrategyImpl implements SidRetrievalStrategy { Collection authorities = this.roleHierarchy .getReachableGrantedAuthorities(authentication.getAuthorities()); List sids = new ArrayList<>(authorities.size() + 1); - sids.add(new PrincipalSid(authentication)); - for (GrantedAuthority authority : authorities) { sids.add(new GrantedAuthoritySid(authority)); } - return sids; } diff --git a/acl/src/main/java/org/springframework/security/acls/domain/SpringCacheBasedAclCache.java b/acl/src/main/java/org/springframework/security/acls/domain/SpringCacheBasedAclCache.java index 37b2bd2fee..0ad9813b5e 100644 --- a/acl/src/main/java/org/springframework/security/acls/domain/SpringCacheBasedAclCache.java +++ b/acl/src/main/java/org/springframework/security/acls/domain/SpringCacheBasedAclCache.java @@ -60,9 +60,7 @@ public class SpringCacheBasedAclCache implements AclCache { @Override public void evictFromCache(Serializable pk) { Assert.notNull(pk, "Primary key (identifier) required"); - MutableAcl acl = getFromCache(pk); - if (acl != null) { this.cache.evict(acl.getId()); this.cache.evict(acl.getObjectIdentity()); @@ -72,9 +70,7 @@ public class SpringCacheBasedAclCache implements AclCache { @Override public void evictFromCache(ObjectIdentity objectIdentity) { Assert.notNull(objectIdentity, "ObjectIdentity required"); - MutableAcl acl = getFromCache(objectIdentity); - if (acl != null) { this.cache.evict(acl.getId()); this.cache.evict(acl.getObjectIdentity()); @@ -98,22 +94,18 @@ public class SpringCacheBasedAclCache implements AclCache { Assert.notNull(acl, "Acl required"); Assert.notNull(acl.getObjectIdentity(), "ObjectIdentity required"); Assert.notNull(acl.getId(), "ID required"); - if ((acl.getParentAcl() != null) && (acl.getParentAcl() instanceof MutableAcl)) { putInCache((MutableAcl) acl.getParentAcl()); } - this.cache.put(acl.getObjectIdentity(), acl); this.cache.put(acl.getId(), acl); } private MutableAcl getFromCache(Object key) { Cache.ValueWrapper element = this.cache.get(key); - if (element == null) { return null; } - return initializeTransientFields((MutableAcl) element.get()); } @@ -122,7 +114,6 @@ public class SpringCacheBasedAclCache implements AclCache { FieldUtils.setProtectedFieldValue("aclAuthorizationStrategy", value, this.aclAuthorizationStrategy); FieldUtils.setProtectedFieldValue("permissionGrantingStrategy", value, this.permissionGrantingStrategy); } - if (value.getParentAcl() != null) { initializeTransientFields((MutableAcl) value.getParentAcl()); } diff --git a/acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java b/acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java index 5d124d8b52..3255b3a967 100644 --- a/acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java +++ b/acl/src/main/java/org/springframework/security/acls/jdbc/AclClassIdUtils.java @@ -70,26 +70,20 @@ class AclClassIdUtils { Serializable identifierFrom(Serializable identifier, ResultSet resultSet) throws SQLException { if (isString(identifier) && hasValidClassIdType(resultSet) && canConvertFromStringTo(classIdTypeFrom(resultSet))) { - - identifier = convertFromStringTo((String) identifier, classIdTypeFrom(resultSet)); - } - else { - // Assume it should be a Long type - identifier = convertToLong(identifier); + return convertFromStringTo((String) identifier, classIdTypeFrom(resultSet)); } - - return identifier; + // Assume it should be a Long type + return convertToLong(identifier); } private boolean hasValidClassIdType(ResultSet resultSet) { - boolean hasClassIdType = false; try { - hasClassIdType = classIdTypeFrom(resultSet) != null; + return classIdTypeFrom(resultSet) != null; } catch (SQLException ex) { log.debug("Unable to obtain the class id type", ex); + return false; } - return hasClassIdType; } private Class classIdTypeFrom(ResultSet resultSet) throws SQLException { @@ -97,16 +91,16 @@ class AclClassIdUtils { } private Class classIdTypeFrom(String className) { - Class targetType = null; - if (className != null) { - try { - targetType = Class.forName(className); - } - catch (ClassNotFoundException ex) { - log.debug("Unable to find class id type on classpath", ex); - } + if (className == null) { + return null; + } + try { + return (Class) Class.forName(className); + } + catch (ClassNotFoundException ex) { + log.debug("Unable to find class id type on classpath", ex); + return null; } - return targetType; } private boolean canConvertFromStringTo(Class targetType) { @@ -128,14 +122,10 @@ class AclClassIdUtils { * @throws IllegalArgumentException if targetType is null */ private Long convertToLong(Serializable identifier) { - Long idAsLong; if (this.conversionService.canConvert(identifier.getClass(), Long.class)) { - idAsLong = this.conversionService.convert(identifier, Long.class); - } - else { - idAsLong = Long.valueOf(identifier.toString()); + return this.conversionService.convert(identifier, Long.class); } - return idAsLong; + return Long.valueOf(identifier.toString()); } private boolean isString(Serializable object) { 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 4ebf564588..9d4d099b25 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 @@ -18,6 +18,7 @@ package org.springframework.security.acls.jdbc; import java.io.Serializable; import java.lang.reflect.Field; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.util.ArrayList; @@ -167,26 +168,19 @@ public class BasicLookupStrategy implements LookupStrategy { } private String computeRepeatingSql(String repeatingSql, int requiredRepetitions) { - assert requiredRepetitions > 0 : "requiredRepetitions must be > 0"; - - final String startSql = this.selectClause; - - final String endSql = this.orderByClause; - + Assert.isTrue(requiredRepetitions > 0, "requiredRepetitions must be > 0"); + String startSql = this.selectClause; + String endSql = this.orderByClause; StringBuilder sqlStringBldr = new StringBuilder( startSql.length() + endSql.length() + requiredRepetitions * (repeatingSql.length() + 4)); sqlStringBldr.append(startSql); - for (int i = 1; i <= requiredRepetitions; i++) { sqlStringBldr.append(repeatingSql); - if (i != requiredRepetitions) { sqlStringBldr.append(" or "); } } - sqlStringBldr.append(endSql); - return sqlStringBldr.toString(); } @@ -228,18 +222,9 @@ public class BasicLookupStrategy implements LookupStrategy { private void lookupPrimaryKeys(final Map acls, final Set findNow, final List sids) { Assert.notNull(acls, "ACLs are required"); Assert.notEmpty(findNow, "Items to find now required"); - String sql = computeRepeatingSql(this.lookupPrimaryKeysWhereClause, findNow.size()); - - Set parentsToLookup = this.jdbcTemplate.query(sql, (ps) -> { - int i = 0; - - for (Long toFind : findNow) { - i++; - ps.setLong(i, toFind); - } - }, new ProcessResultSet(acls, sids)); - + Set parentsToLookup = this.jdbcTemplate.query(sql, (ps) -> setKeys(ps, findNow), + new ProcessResultSet(acls, sids)); // Lookup the parents, now that our JdbcTemplate has released the database // connection (SEC-547) if (parentsToLookup.size() > 0) { @@ -247,6 +232,14 @@ public class BasicLookupStrategy implements LookupStrategy { } } + private void setKeys(PreparedStatement ps, Set findNow) throws SQLException { + int i = 0; + for (Long toFind : findNow) { + i++; + ps.setLong(i, toFind); + } + } + /** * The main method. *

@@ -269,68 +262,48 @@ public class BasicLookupStrategy implements LookupStrategy { public final Map readAclsById(List objects, List sids) { Assert.isTrue(this.batchSize >= 1, "BatchSize must be >= 1"); Assert.notEmpty(objects, "Objects to lookup required"); - // Map - Map result = new HashMap<>(); // contains - // FULLY - // loaded - // Acl - // objects - + // contains FULLY loaded Acl objects + Map result = new HashMap<>(); Set currentBatchToLoad = new HashSet<>(); - for (int i = 0; i < objects.size(); i++) { final ObjectIdentity oid = objects.get(i); boolean aclFound = false; - // Check we don't already have this ACL in the results if (result.containsKey(oid)) { aclFound = true; } - // Check cache for the present ACL entry if (!aclFound) { Acl acl = this.aclCache.getFromCache(oid); - // Ensure any cached element supports all the requested SIDs // (they should always, as our base impl doesn't filter on SID) if (acl != null) { - if (acl.isSidLoaded(sids)) { - result.put(acl.getObjectIdentity(), acl); - aclFound = true; - } - else { - throw new IllegalStateException( - "Error: SID-filtered element detected when implementation does not perform SID filtering " - + "- have you added something to the cache manually?"); - } + Assert.state(acl.isSidLoaded(sids), + "Error: SID-filtered element detected when implementation does not perform SID filtering " + + "- have you added something to the cache manually?"); + result.put(acl.getObjectIdentity(), acl); + aclFound = true; } } - // Load the ACL from the database if (!aclFound) { currentBatchToLoad.add(oid); } - // Is it time to load from JDBC the currentBatchToLoad? if ((currentBatchToLoad.size() == this.batchSize) || ((i + 1) == objects.size())) { if (currentBatchToLoad.size() > 0) { Map loadedBatch = lookupObjectIdentities(currentBatchToLoad, sids); - // Add loaded batch (all elements 100% initialized) to results result.putAll(loadedBatch); - // Add the loaded batch to the cache - for (Acl loadedAcl : loadedBatch.values()) { this.aclCache.putInCache((AclImpl) loadedAcl); } - currentBatchToLoad.clear(); } } } - return result; } @@ -343,37 +316,20 @@ public class BasicLookupStrategy implements LookupStrategy { *

* This subclass is required to return fully valid Acls, including * properly-configured parent ACLs. - * */ private Map lookupObjectIdentities(final Collection objectIdentities, List sids) { Assert.notEmpty(objectIdentities, "Must provide identities to lookup"); - final Map acls = new HashMap<>(); // contains - // Acls - // with - // StubAclParents + // contains Acls with StubAclParents + Map acls = new HashMap<>(); // Make the "acls" map contain all requested objectIdentities // (including markers to each parent in the hierarchy) String sql = computeRepeatingSql(this.lookupObjectIdentitiesWhereClause, objectIdentities.size()); - Set parentsToLookup = this.jdbcTemplate.query(sql, (ps) -> { - int i = 0; - for (ObjectIdentity oid : objectIdentities) { - // Determine prepared statement values for this iteration - String type = oid.getType(); - - // No need to check for nulls, as guaranteed non-null by - // ObjectIdentity.getIdentifier() interface contract - String identifier = oid.getIdentifier().toString(); - - // Inject values - ps.setString((2 * i) + 1, identifier); - ps.setString((2 * i) + 2, type); - i++; - } - }, new ProcessResultSet(acls, sids)); + Set parentsToLookup = this.jdbcTemplate.query(sql, + (ps) -> setupLookupObjectIdentitiesStatement(ps, objectIdentities), new ProcessResultSet(acls, sids)); // Lookup the parents, now that our JdbcTemplate has released the database // connection (SEC-547) @@ -383,11 +339,9 @@ public class BasicLookupStrategy implements LookupStrategy { // Finally, convert our "acls" containing StubAclParents into true Acls Map resultMap = new HashMap<>(); - for (Acl inputAcl : acls.values()) { Assert.isInstanceOf(AclImpl.class, inputAcl, "Map should have contained an AclImpl"); Assert.isInstanceOf(Long.class, ((AclImpl) inputAcl).getId(), "Acl.getId() must be Long"); - Acl result = convert(acls, (Long) ((AclImpl) inputAcl).getId()); resultMap.put(result.getObjectIdentity(), result); } @@ -395,6 +349,24 @@ public class BasicLookupStrategy implements LookupStrategy { return resultMap; } + private void setupLookupObjectIdentitiesStatement(PreparedStatement ps, Collection objectIdentities) + throws SQLException { + int i = 0; + for (ObjectIdentity oid : objectIdentities) { + // Determine prepared statement values for this iteration + String type = oid.getType(); + + // No need to check for nulls, as guaranteed non-null by + // ObjectIdentity.getIdentifier() interface contract + String identifier = oid.getIdentifier().toString(); + + // Inject values + ps.setString((2 * i) + 1, identifier); + ps.setString((2 * i) + 2, type); + i++; + } + } + /** * The final phase of converting the Map of AclImpl * instances which contain StubAclParents into proper, valid @@ -402,7 +374,6 @@ public class BasicLookupStrategy implements LookupStrategy { * @param inputMap the unconverted AclImpls * @param currentIdentity the currentAcl that we wish to convert (this * may be - * */ private AclImpl convert(Map inputMap, Long currentIdentity) { Assert.notEmpty(inputMap, "InputMap required"); @@ -460,9 +431,7 @@ public class BasicLookupStrategy implements LookupStrategy { if (isPrincipal) { return new PrincipalSid(sid); } - else { - return new GrantedAuthoritySid(sid); - } + return new GrantedAuthoritySid(sid); } /** @@ -564,7 +533,6 @@ public class BasicLookupStrategy implements LookupStrategy { // Now try to find it in the cache MutableAcl cached = BasicLookupStrategy.this.aclCache.getFromCache(parentId); - if ((cached == null) || !cached.isSidLoaded(this.sids)) { parentIdsToLookup.add(parentId); } 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 f78ec7601f..935466f5d1 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 @@ -17,6 +17,8 @@ package org.springframework.security.acls.jdbc; import java.io.Serializable; +import java.sql.ResultSet; +import java.sql.SQLException; import java.util.Collections; import java.util.List; import java.util.Map; @@ -94,18 +96,16 @@ public class JdbcAclService implements AclService { @Override public List findChildren(ObjectIdentity parentIdentity) { Object[] args = { parentIdentity.getIdentifier().toString(), parentIdentity.getType() }; - List objects = this.jdbcOperations.query(this.findChildrenSql, args, (rs, rowNum) -> { - String javaType = rs.getString("class"); - Serializable identifier = (Serializable) rs.getObject("obj_id"); - identifier = this.aclClassIdUtils.identifierFrom(identifier, rs); - return new ObjectIdentityImpl(javaType, identifier); - }); - - if (objects.isEmpty()) { - return null; - } + List objects = this.jdbcOperations.query(this.findChildrenSql, args, + (rs, rowNum) -> mapObjectIdentityRow(rs)); + return (!objects.isEmpty()) ? objects : null; + } - return objects; + 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); } @Override @@ -113,7 +113,6 @@ public class JdbcAclService implements AclService { 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); } @@ -131,7 +130,6 @@ public class JdbcAclService implements AclService { 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) { @@ -139,7 +137,6 @@ public class JdbcAclService implements AclService { throw new NotFoundException("Unable to find ACL information for object identity '" + oid + "'"); } } - return result; } diff --git a/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcMutableAclService.java b/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcMutableAclService.java index eb52374c07..f1c7a7a174 100644 --- a/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcMutableAclService.java +++ b/acl/src/main/java/org/springframework/security/acls/jdbc/JdbcMutableAclService.java @@ -139,6 +139,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS return; } this.jdbcOperations.batchUpdate(this.insertEntry, new BatchPreparedStatementSetter() { + @Override public int getBatchSize() { return acl.getEntries().size(); @@ -158,6 +159,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS stmt.setBoolean(6, entry.isAuditSuccess()); stmt.setBoolean(7, entry.isAuditFailure()); } + }); } @@ -216,22 +218,15 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS */ protected Long createOrRetrieveSidPrimaryKey(Sid sid, boolean allowCreate) { Assert.notNull(sid, "Sid required"); - - String sidName; - boolean sidIsPrincipal = true; - if (sid instanceof PrincipalSid) { - sidName = ((PrincipalSid) sid).getPrincipal(); + String sidName = ((PrincipalSid) sid).getPrincipal(); + return createOrRetrieveSidPrimaryKey(sidName, true, allowCreate); } - else if (sid instanceof GrantedAuthoritySid) { - sidName = ((GrantedAuthoritySid) sid).getGrantedAuthority(); - sidIsPrincipal = false; + if (sid instanceof GrantedAuthoritySid) { + String sidName = ((GrantedAuthoritySid) sid).getGrantedAuthority(); + return createOrRetrieveSidPrimaryKey(sidName, false, allowCreate); } - else { - throw new IllegalArgumentException("Unsupported implementation of Sid"); - } - - return createOrRetrieveSidPrimaryKey(sidName, sidIsPrincipal, allowCreate); + throw new IllegalArgumentException("Unsupported implementation of Sid"); } /** @@ -243,20 +238,16 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS * @return the primary key or null if not found */ protected Long createOrRetrieveSidPrimaryKey(String sidName, boolean sidIsPrincipal, boolean allowCreate) { - List sidIds = this.jdbcOperations.queryForList(this.selectSidPrimaryKey, new Object[] { sidIsPrincipal, sidName }, Long.class); - if (!sidIds.isEmpty()) { return sidIds.get(0); } - if (allowCreate) { this.jdbcOperations.update(this.insertSid, sidIsPrincipal, sidName); Assert.isTrue(TransactionSynchronizationManager.isSynchronizationActive(), "Transaction must be running"); return this.jdbcOperations.queryForObject(this.sidIdentityQuery, Long.class); } - return null; } @@ -264,7 +255,6 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS public void deleteAcl(ObjectIdentity objectIdentity, boolean deleteChildren) throws ChildrenExistException { Assert.notNull(objectIdentity, "Object Identity required"); Assert.notNull(objectIdentity.getIdentifier(), "Object Identity doesn't provide an identifier"); - if (deleteChildren) { List children = findChildren(objectIdentity); if (children != null) { @@ -276,8 +266,7 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS else { if (!this.foreignKeysInDatabase) { // We need to perform a manual verification for what a FK would normally - // do - // We generally don't do this, in the interests of deadlock management + // do. We generally don't do this, in the interests of deadlock management List children = findChildren(objectIdentity); if (children != null) { throw new ChildrenExistException( @@ -384,21 +373,16 @@ public class JdbcMutableAclService extends JdbcAclService implements MutableAclS */ protected void updateObjectIdentity(MutableAcl acl) { Long parentId = null; - if (acl.getParentAcl() != null) { Assert.isInstanceOf(ObjectIdentityImpl.class, acl.getParentAcl().getObjectIdentity(), "Implementation only supports ObjectIdentityImpl"); - ObjectIdentityImpl oii = (ObjectIdentityImpl) acl.getParentAcl().getObjectIdentity(); parentId = retrieveObjectIdentityPrimaryKey(oii); } - Assert.notNull(acl.getOwner(), "Owner is required in this implementation"); - Long ownerSid = createOrRetrieveSidPrimaryKey(acl.getOwner(), true); int count = this.jdbcOperations.update(this.updateObjectIdentity, parentId, ownerSid, acl.isEntriesInheriting(), acl.getId()); - if (count != 1) { throw new NotFoundException("Unable to locate ACL to update"); } diff --git a/acl/src/main/java/org/springframework/security/acls/model/AccessControlEntry.java b/acl/src/main/java/org/springframework/security/acls/model/AccessControlEntry.java index 90a31179d7..d8c8e286f6 100644 --- a/acl/src/main/java/org/springframework/security/acls/model/AccessControlEntry.java +++ b/acl/src/main/java/org/springframework/security/acls/model/AccessControlEntry.java @@ -27,7 +27,6 @@ import java.io.Serializable; *

* * @author Ben Alex - * */ public interface AccessControlEntry extends Serializable { diff --git a/acl/src/main/java/org/springframework/security/acls/model/AclCache.java b/acl/src/main/java/org/springframework/security/acls/model/AclCache.java index 461d742c80..945b1b1a2a 100644 --- a/acl/src/main/java/org/springframework/security/acls/model/AclCache.java +++ b/acl/src/main/java/org/springframework/security/acls/model/AclCache.java @@ -24,7 +24,6 @@ import org.springframework.security.acls.jdbc.JdbcAclService; * A caching layer for {@link JdbcAclService}. * * @author Ben Alex - * */ public interface AclCache { diff --git a/acl/src/main/java/org/springframework/security/acls/model/AuditableAccessControlEntry.java b/acl/src/main/java/org/springframework/security/acls/model/AuditableAccessControlEntry.java index eac37798a1..e14a5d1596 100644 --- a/acl/src/main/java/org/springframework/security/acls/model/AuditableAccessControlEntry.java +++ b/acl/src/main/java/org/springframework/security/acls/model/AuditableAccessControlEntry.java @@ -20,7 +20,6 @@ package org.springframework.security.acls.model; * Represents an ACE that provides auditing information. * * @author Ben Alex - * */ public interface AuditableAccessControlEntry extends AccessControlEntry { diff --git a/acl/src/main/java/org/springframework/security/acls/model/AuditableAcl.java b/acl/src/main/java/org/springframework/security/acls/model/AuditableAcl.java index e83f298777..3760d78dc3 100644 --- a/acl/src/main/java/org/springframework/security/acls/model/AuditableAcl.java +++ b/acl/src/main/java/org/springframework/security/acls/model/AuditableAcl.java @@ -20,7 +20,6 @@ package org.springframework.security.acls.model; * A mutable ACL that provides audit capabilities. * * @author Ben Alex - * */ public interface AuditableAcl extends MutableAcl { diff --git a/acl/src/main/java/org/springframework/security/acls/model/ObjectIdentityRetrievalStrategy.java b/acl/src/main/java/org/springframework/security/acls/model/ObjectIdentityRetrievalStrategy.java index f39c58fe2e..3838443a5d 100644 --- a/acl/src/main/java/org/springframework/security/acls/model/ObjectIdentityRetrievalStrategy.java +++ b/acl/src/main/java/org/springframework/security/acls/model/ObjectIdentityRetrievalStrategy.java @@ -21,7 +21,6 @@ package org.springframework.security.acls.model; * will be returned for a particular domain object * * @author Ben Alex - * */ public interface ObjectIdentityRetrievalStrategy {