SEC-951: Acl Serialization Errors that cohere with parent-child-structure of Acls. #1207

Closed
spring-issuemaster opened this Issue Aug 9, 2008 · 6 comments

Comments

Projects
None yet
1 participant
from SEC-951) said:

I found 2 bugs that cohere with a parent-child-structure of the acls.
1. Bug
the serialization problems occur because the object graph that is passed to the cache contains Objects the are not serializable:
the error log contians the " ‘org.springframework.security.acls.jdbc.BasicLookupStrategy’ not serializable"- exception. so i wondered how this class can be part of the object graph. The answer is: The AclImpl still contains references to the private class StubAclParent that is an inner class of org.springframework.security.acls.jdbc.BasicLookupStrategy. That is the link between the serialization problems and the " ‘org.springframework.security.acls.jdbc.BasicLookupStrategy’ not serializable"- exception.

How can that happen?

It is the job of the convert method to replace the stubaclparents by real acls. But this method does not work properly:

The acl-field of the aces still points to an unreal AclImpl.

to fix this the convert method could be changed like this

```
private AclImpl convert(Map inputMap, Long currentIdentity) throws IllegalArgumentException, IllegalAccessException {
Assert.notEmpty(inputMap, “InputMap required”);
Assert.notNull(currentIdentity, “CurrentIdentity required”);

// Retrieve this Acl from the InputMap Acl uncastAcl = (Acl) inputMap.get(currentIdentity); Assert.isInstanceOf(AclImpl.class, uncastAcl, “The inputMap contained a non-AclImpl”); AclImpl inputAcl = (AclImpl) uncastAcl; Acl parent = inputAcl.getParentAcl(); if ((parent != null) && parent instanceof StubAclParent) { // Lookup the parent StubAclParent stubAclParent = (StubAclParent) parent; parent = convert(inputMap, stubAclParent.getId()); } // Now we have the parent (if there is one), create the true AclImpl AclImpl result = new AclImpl(inputAcl.getObjectIdentity(), (Long) inputAcl.getId(), aclAuthorizationStrategy, auditLogger, parent, null, inputAcl.isEntriesInheriting(), inputAcl.getOwner()); // Copy the “aces” from the input to the destination Field fieldAces = FieldUtils.getField(AclImpl.class, “aces”); //try { fieldAces.setAccessible(true); List aces = (List) fieldAces.get(inputAcl); List acesN = new Vector(); Iterator i = aces.iterator();

```

// replace the old aclImpl (that contains StubAclParents) by the new one.
while(i.hasNext()) {
AccessControlEntryImpl ace = (AccessControlEntryImpl) i.next();
Field fieldAcl = FieldUtils.getField(AccessControlEntryImpl.class, “acl”);
fieldAcl.setAccessible(true);
fieldAcl.set(ace, result);
acesNew.add(ace);
}
fieldAces.set(result, acesNew);
//} catch (IllegalAccessException ex) {
//throw new IllegalStateException(“Could not obtain or set AclImpl.ace field”);
//}

```
return result;
}
```
1. Bug

EhCacheBasedAclCache does not initialize the transient fields of the parent acls which causes nullpointerexceptions.

Ben Alex said:

Modified BasicLookupStrategy with suggested fix. Tests pass. Checked in as SVN revision 3270.

Marking this issue as “resolved” as opposed to “closed”, but to the difficulty in reproducing it. Would those who originally reported this issue (and the SEC-527) kindly comment this issue to confirm the issue is resolved in Spring Security 2.0.4.

Emanuel said:

EhCacheBasedAclCache does not initialize the transient fields of the parent acls which causes nullpointerexceptions.

the method ‘initializetransientfields’ has to call itself to initialize the transient fields of each parentacl.

```
private MutableAcl initializeTransientFields(MutableAcl value) {
if (value instanceof AclImpl) {
FieldUtils.setProtectedFieldValue(“aclAuthorizationStrategy”, value, this.aclAuthorizationStrategy);
FieldUtils.setProtectedFieldValue(“auditLogger”, value, this.auditLogger);
if(value.getParentAcl() != null) {
initializeTransientFields((MutableAcl) value.getParentAcl());
}
}
return value;
}
```

Ben Alex said:

Does this remain a problem in 2.0.4?

Emanuel said:

Yes it does.
the method ‘initializetransientfields’ has to call itself to initialize the transient fields of each parentacl!

The Method can be changed like this:

```
private MutableAcl initializeTransientFields(MutableAcl value) {
if (value instanceof AclImpl) {
FieldUtils.setProtectedFieldValue(“aclAuthorizationStrategy”, value, this.aclAuthorizationStrategy);
FieldUtils.setProtectedFieldValue(“auditLogger”, value, this.auditLogger);
if(value.getParentAcl() != null) {
initializeTransientFields((MutableAcl) value.getParentAcl());
}
}
return value;
```

}

Luke Taylor said:

I've modified the tests to reproduce this issue. This was quite tricky as you have to make sure that Ehcache isn't returning a the value from its diskstore spool, rather than the disk. Ehcache writes the spool asynchronously in another thread and the test may read back the value from the spool rather than getting a de-serialized copy.

I've applied the suggested patch (a recursive call on the parent Acl).

spring-issuemaster added this to the 3.0.0 RC1 milestone Feb 5, 2016

This issue is related to #788

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment