Spring Security
  1. Spring Security
  2. SEC-1042

Simplify AclService configuration based on default Spring implementations of required service

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.4
    • Fix Version/s: 3.1.0.M1
    • Component/s: ACLs
    • Labels:
      None

      Description

      If you look at the Contacts sample, in the configuration file (samples/contacts/src/main/resources/applicationContext-common-authorization.xml), it declares the aclService bean. The JdbcMutableAclService class requires passing LookupStrategy and AclCache implementations, which makes the file very complicated - see:

      <bean id="aclCache" class="org.springframework.security.acls.jdbc.EhCacheBasedAclCache">
      <constructor-arg>
      <bean class="org.springframework.cache.ehcache.EhCacheFactoryBean">
      <property name="cacheManager">
      <bean class="org.springframework.cache.ehcache.EhCacheManagerFactoryBean"/>
      </property>
      <property name="cacheName" value="aclCache"/>
      </bean>
      </constructor-arg>
      </bean>

      <bean id="lookupStrategy" class="org.springframework.security.acls.jdbc.BasicLookupStrategy">
      <constructor-arg ref="dataSource"/>
      <constructor-arg ref="aclCache"/>
      <constructor-arg>
      <bean class="org.springframework.security.acls.domain.AclAuthorizationStrategyImpl">
      <constructor-arg>
      <list>
      <bean class="org.springframework.security.GrantedAuthorityImpl">
      <constructor-arg value="ROLE_ADMINISTRATOR"/>
      </bean>
      <bean class="org.springframework.security.GrantedAuthorityImpl">
      <constructor-arg value="ROLE_ADMINISTRATOR"/>
      </bean>
      <bean class="org.springframework.security.GrantedAuthorityImpl">
      <constructor-arg value="ROLE_ADMINISTRATOR"/>
      </bean>
      </list>
      </constructor-arg>
      </bean>
      </constructor-arg>
      <constructor-arg>
      <bean class="org.springframework.security.acls.domain.ConsoleAuditLogger"/>
      </constructor-arg>
      </bean>

      <bean id="aclService" class="org.springframework.security.acls.jdbc.JdbcMutableAclService">
      <constructor-arg ref="dataSource"/>
      <constructor-arg ref="lookupStrategy"/>
      <constructor-arg ref="aclCache"/>
      </bean>

      Most of this configures some services, that have only one implementation OOTB in SpringSecurity: AclCache, LookupStrategy, AclAuthorizationStrategy, AuditLogger. If user doesn't create own implementations of those service, he has to always copy this big part of configuration. The only thing that can change is the reference do data source, and the name of roles for AclAutorizationStrategii. (BTW. is it an error in the sample? why ADMINISTRATOR_ROLE is declared three times?). So it would be better if we provide simplified constructor for JdbcMutableService, taking only dataSource and the granted authorities array, and using the default implementations:

      <bean id="aclService" class="org.springframework.security.acls.jdbc.JdbcMutableAclService">
      <constructor-arg ref="dataSource"/>
      <constructor-arg>
      <list>
      <bean class="org.springframework.security.GrantedAuthorityImpl">
      <constructor-arg value="ROLE_ADMINISTRATOR"/>
      </bean>
      </list>
      </constructor-arg>
      </bean>

      It looks much better, doesn't it?

      1. SEC-1042.patch
        6 kB
        Grzegorz Borkowski

        Activity

        Hide
        Grzegorz Borkowski added a comment -

        I attach the patch with possible implementation. It is a bit hacky though, because of the fact that the constructor must call the super constructor providing LookupStategy, which has the dependency on AclCache, which is used by the AclService later too.

        Show
        Grzegorz Borkowski added a comment - I attach the patch with possible implementation. It is a bit hacky though, because of the fact that the constructor must call the super constructor providing LookupStategy, which has the dependency on AclCache, which is used by the AclService later too.
        Hide
        Grzegorz Borkowski added a comment -

        After reanalyzing this proposal and discussing it with some people, I came to the conclusion that adding the simplified constructor is not the way to go. There are several weaknesses of this idea:
        -Implementation proposed in my patch is too much hacky: it is too invasive in other classes (JdbcAclService and BasicLookupStrategy), it breaks encapsulation even if you use getters instead of direct field access; both field and method access are only of package-level fortunately, but still I don't like it;

        • Alternative solution is to use some temporary static field in JdbcMutableAclService to hold the AclCache for a while, but still it is not the best pattern;
        • The big problem is that the objects created (EhCacheBasedAclCache, EhCacheFactoryBean, CacheManager, EhCacheManagerFactoryBean, BasicLookupStrategy, AclAuthorizationStrategyImpl, ...) won't be registered and instantiated in Spring context. This may be or may be not an issue, but even if it is not now, it may suddenly become problematic later if the implementation of any of this class changes. I think that common approach is to treat those beans as Spring-managed beans.

        I think that the standard Spring approach to such situation is to replace declaring many "infrastructure", inter-related bean patterns in XML, by the XML-namaspace tag. So this is I think the better way to go: we may replace the whole code that I past in initial comment with something similar to this:

        <security:mutableAclService data-source-ref="dataSource" authorization-strategy-ref="authorizationStrategy"/>

        <bean id="authorizationStrategy" class="org.springframework.security.acls.domain.AclAuthorizationStrategyImpl">
        <constructor-arg>
        <list>
        <bean class="org.springframework.security.GrantedAuthorityImpl">
        <constructor-arg value="ROLE_ADMINISTRATOR"/>
        </bean>
        </list>
        </constructor-arg>
        </bean>

        Show
        Grzegorz Borkowski added a comment - After reanalyzing this proposal and discussing it with some people, I came to the conclusion that adding the simplified constructor is not the way to go. There are several weaknesses of this idea: -Implementation proposed in my patch is too much hacky: it is too invasive in other classes (JdbcAclService and BasicLookupStrategy), it breaks encapsulation even if you use getters instead of direct field access; both field and method access are only of package-level fortunately, but still I don't like it; Alternative solution is to use some temporary static field in JdbcMutableAclService to hold the AclCache for a while, but still it is not the best pattern; The big problem is that the objects created (EhCacheBasedAclCache, EhCacheFactoryBean, CacheManager, EhCacheManagerFactoryBean, BasicLookupStrategy, AclAuthorizationStrategyImpl, ...) won't be registered and instantiated in Spring context. This may be or may be not an issue, but even if it is not now, it may suddenly become problematic later if the implementation of any of this class changes. I think that common approach is to treat those beans as Spring-managed beans. I think that the standard Spring approach to such situation is to replace declaring many "infrastructure", inter-related bean patterns in XML, by the XML-namaspace tag. So this is I think the better way to go: we may replace the whole code that I past in initial comment with something similar to this: <security:mutableAclService data-source-ref="dataSource" authorization-strategy-ref="authorizationStrategy"/> <bean id="authorizationStrategy" class="org.springframework.security.acls.domain.AclAuthorizationStrategyImpl"> <constructor-arg> <list> <bean class="org.springframework.security.GrantedAuthorityImpl"> <constructor-arg value="ROLE_ADMINISTRATOR"/> </bean> </list> </constructor-arg> </bean>
        Hide
        Luke Taylor added a comment -

        I agree that we need to add namespace configuration for ACLs at some point, but it isn't something that we want to rush into without more discussion. The ACL package itself may undergo some changes for the next version and it will also be affected by the move towards favouring the use of expressions in security metadata. So I'm scheduling this for 2.6 for the time being.

        Show
        Luke Taylor added a comment - I agree that we need to add namespace configuration for ACLs at some point, but it isn't something that we want to rush into without more discussion. The ACL package itself may undergo some changes for the next version and it will also be affected by the move towards favouring the use of expressions in security metadata. So I'm scheduling this for 2.6 for the time being.
        Hide
        Luke Taylor added a comment -

        Closing for now, as implementing namespace support for ACL isn't a major priority. The configuration is also simpler since the use of expressions has been introduced. See also SEC-1533 which simplifies the constructor for AclAuthorizationStrategyImpl.

        Show
        Luke Taylor added a comment - Closing for now, as implementing namespace support for ACL isn't a major priority. The configuration is also simpler since the use of expressions has been introduced. See also SEC-1533 which simplifies the constructor for AclAuthorizationStrategyImpl.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Grzegorz Borkowski
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: