Spring Security
  1. Spring Security
  2. SEC-951

Acl Serialization Errors that cohere with parent-child-structure of Acls.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0 M1, 2.0.0 M2, 2.0.0 RC1, 2.0.0, 2.0.1, 2.0.2, 2.0.3
    • Fix Version/s: 3.0.0 RC1
    • Component/s: ACLs
    • Labels:
      None

      Description

      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;
      }

      2. Bug

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

        Issue Links

          Activity

          Hide
          Ben Alex added a comment -

          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.

          Show
          Ben Alex added a comment - 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.
          Hide
          Emanuel added a comment -

          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;
          }

          Show
          Emanuel added a comment - 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; }
          Hide
          Ben Alex added a comment -

          Does this remain a problem in 2.0.4?

          Show
          Ben Alex added a comment - Does this remain a problem in 2.0.4?
          Hide
          Emanuel added a comment -

          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;
          }

          Show
          Emanuel added a comment - 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; }
          Hide
          Luke Taylor added a comment -

          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).

          Show
          Luke Taylor added a comment - 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).

            People

            • Assignee:
              Luke Taylor
              Reporter:
              Emanuel
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: