SEC-1042: Simplify AclService configuration based on default Spring implementations of required service #1293

Closed
spring-issuemaster opened this Issue Nov 27, 2008 · 4 comments

1 participant

@spring-issuemaster

Grzegorz Borkowski (Migrated from SEC-1042) said:

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:

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:

It looks much better, doesn’t it?

@spring-issuemaster

Grzegorz Borkowski said:

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.

@spring-issuemaster

Grzegorz Borkowski said:

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:

@spring-issuemaster

Luke Taylor said:

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.

@spring-issuemaster

Luke Taylor said:

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.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.M1 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment