SEC-1557: add a getter method to DelegatingMethodSecurityMetadataSource #1798

Closed
spring-issuemaster opened this Issue Sep 4, 2010 · 12 comments

1 participant

@spring-issuemaster

Wolfgang Winter (Migrated from SEC-1557) said:

Hi,

For the next release of Cibet control framework (www.logitags.com/cibet) I have integrated Spring Security authorization. It was possible but I had to do some 'dirty' hacks which could be avoided if Spring Security provides some very small changes:

I exchange the DelegatingMethodSecurityMetadataSource in MethodSecurityInterceptor with an own implementation CibetDelegatingMethodSecurityMetadataSource. My implementation must work also with standard Spring Security authorization. Therefore I need to set the list of MethodSecurityMetadataSource from DelegatingMethodSecurityMetadataSource into my CibetDelegatingMethodSecurityMetadataSource.

Problem: Not possible because getter is missing.
Workaround: I use field access reflection on private field methodSecurityMetadataSources (dirty hack)
Solution: add a getter to DelegatingMethodSecurityMetadataSource:

public List getMethodSecurityMetadataSources() {
return methodSecurityMetadataSources;
}

@spring-issuemaster

Luke Taylor said:

Could you provide a pointer to the code you're referring to please? I don't see any Spring Security dependency in

https://cibet.svn.sourceforge.net/svnroot/cibet/trunk

@spring-issuemaster

Wolfgang Winter said:

The code is not yet published and sources are not any more in sourceforge. Spring Security integration will be in version 0.8 and will be released in the next few weeks (still some documentation to write). I add the springsecurity package which contains all integration classes.

@spring-issuemaster

Luke Taylor said:

I don't have a complete overview of how this is intended to work, but looking at SpringSecurityActuator, it reads the list of MethodSecurityMetadataSource instances created by Spring Security, but it then iterates through them and calls DelegatingMethodSecurityMetadataSource.setCibetMetadataSource() using the first one it finds. So it seems that the behaviour will differ depending on the ordering in the list, and will only support one type of metadata. Is this intentional?

@spring-issuemaster

Wolfgang Winter said:

The goal for me is: With Cibet Spring Security must still work like it works stand-alone, the Cibet integration is additional
functionality that should not change behaviour to users who have experience with SpSe. I exchange DelegatingMethodSecurityMetadataSource with CibetDelegatingMethodSecurityMetadataSource in MethodSecurityInterceptor. The Cibet version must work like DelegatingMethodSecurityMetadataSource so I copy the list of MethodSecurityMetadataSource which will be used for standard Spring Security configurations (Spring config, annotations etc.) Additionally, security configuration can be done in Cibet Setpoints and this uses only the MethodSecurityMetadataSource instance in cibetDel.cibetMetadataSource field.

Maybe I don't understand the inner working of Spring Security correctly but I suppose at least one of the instances in the
list of MethodSecurityMetadataSource will be of type PrePostAnnotationSecurityMetadataSource, SecuredAnnotationSecurityMetadataSource or Jsr250MethodSecurityMetadataSource depending on the
configuration and only one of these three at a time. Spring documentation says: You should only declare one element.
If this is true, the ordering doesn't matter for me.

@spring-issuemaster

Luke Taylor said:

The assumption that only one of the metadata types can be enabled at a time is false. That's why I was commenting on the initialization code in your SpringSecurityActuator, which makes this assumption. An application could use both @Secured annotations and expressions, for example.

@spring-issuemaster

Wolfgang Winter said:

I see. I suppose something like this is possible?
secured-annotations="enabled" pre-post-annotations="enabled" />

That was not clear for me from the docu. Thanks for the hint. I corrected this in SpringSecurityActuator and
CibetDelegatingMethodSecurityMetadataSource (see attached sources).

But when I read the SS code correctly the following code will not react as expected maybe :

@Secured("ROLE_TELLER")
@PreAuthorize("hasPermission(#contact, 'admin')")
public void deletePermission(Contact contact, Sid recipient, Permission permission);

This annotations may express a legal requirement: User must have role TELLER and admin permission for contact.
In this case not both annotations are evaluated. It depends on the ordering in
DelegatingMethodSecurityMetadataSource.methodSecurityMetadataSources
which one is taken.

@spring-issuemaster

Luke Taylor said:

It could perhaps be clearer, but the docs on global-method-security say "You can enable more than one type of annotation in the same application, but you should avoid mixing annotations types in the same interface or class to avoid confusion." The annotated types should be clearly segregated or the behaviour would not be defined. This could be useful when it becomes clear that some part of an app needs to support more complex authorization, or when using a separately created module.

@spring-issuemaster

Wolfgang Winter said:

Release 0.8 with Spring Security integration has been released. Please check the web site.
What do you think about my requirements for one of the next releases of Spring Security? My hacks are not very nice and I also feel not good that I am forced to have Cibet classes in the Spring Security package scope.
best regards
Wolfgang

@spring-issuemaster

Luke Taylor said:

Your requirements give us a chance to reassess some of the design choices and possibly come up with better alternatives. However, I'm never keen to just take the option which provides a fix for one use case, especially if it involves exposing framework internals which have not previously been public. Adding a getter to this class isn't a big issue, but encouraging the use of inheritance or making package-private classes public is a different matter since it encourages tighter coupling with our code and makes maintenance and modification more difficult. I'd prefer to come up with better alternatives where possible, but maintainability of the framework code will always take precedence over facilitating reuse in app code.

@spring-issuemaster

Wolfgang Winter said:

I understand your reluctance about inheritance and public/private. The general use case would be to allow for an easier possibility of other metadata sources as you mentioned in #1558. Maybe you have a better design idea to achieve this.

@spring-issuemaster

Luke Taylor said:

I've added the getter method as requested.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.M2 milestone Feb 5, 2016
@spring-issuemaster

This issue relates to #1799

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