Spring Security
  1. Spring Security
  2. SEC-1198

MethodSecurityPriviligeEvaluator Returns True When Principal Does Not Have Required Role

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 2.0.5
    • Fix Version/s: 3.0.0 M2
    • Component/s: Core
    • Labels:
      None

      Description

      We've implemented an IAuthorizationService.isCallable(Object object, String methodName) service.

      The isCallable method looks up the object's security interceptor in a map and then sets the security interceptor on the MethodInvocationPriviliegeEvaluator instance. MethodInvocationPriviliegeEvaluator is then called to see whether the method invocation is allowed.

      True is returned regardless of whether the authentication has the required role or not.

      I'm attaching a maven project with all source code and tests.

      The test to look at is:
      spring.security.jira/src/test/java/com/example/security/authorization/tests/AuthorizationServiceTest.java

      SIDE NOTE:
      I believe the method MethodInvocationPriviliegeEvaluator currently requires a MethodSecurityInterceptor instance for it's initialization. This required us to wire in "Dummy" classes just to get the container to start up. When isCallable is called, we replace the "Dummy" MethodSecurityInterceptor instance with the instance the container created for the secured object.

        Activity

        Hide
        Luke Taylor added a comment -

        I've had a look at your code and I believe this is because of the way your AuthorizationServiceImpl does the method lookups. The SecurityMetadataSource has the attributes registered against the interface INeedsMethodAuthorization, and to get a match, the Method object you use to create the SimpleMethodInvocation would have to also be on the interface. In fact, the object you are invoking the method on is a Java dynamic proxy, so the class is of this type.

        You can cast the object to "Advised", then use getTargetSource().getTarget() to get the actual target object.

        Similarly you can call getProxiedInterfaces() and check through these for the method name in order to locate the method (if you want to register it by interface). If to check for metadata registered against the the implementation, then you can call getClass() method on the actual target object.

        Show
        Luke Taylor added a comment - I've had a look at your code and I believe this is because of the way your AuthorizationServiceImpl does the method lookups. The SecurityMetadataSource has the attributes registered against the interface INeedsMethodAuthorization, and to get a match, the Method object you use to create the SimpleMethodInvocation would have to also be on the interface. In fact, the object you are invoking the method on is a Java dynamic proxy, so the class is of this type. You can cast the object to "Advised", then use getTargetSource().getTarget() to get the actual target object. Similarly you can call getProxiedInterfaces() and check through these for the method name in order to locate the method (if you want to register it by interface). If to check for metadata registered against the the implementation, then you can call getClass() method on the actual target object.
        Hide
        Ole Ersoy added a comment -

        Yes! Luke thanks a gazillion for explaining that. I gave it a shot and I think it works fine now, both for interface based security metadata and implementation based security meta data. I added a property to the service called implementationBasedSecurityMetaData, which defaults to true. If it's set to false, then the service expects the meta data to be keyed on an interface.

        I'll upload the project with the modified service and test cases next.

        Thanks again!

        Show
        Ole Ersoy added a comment - Yes! Luke thanks a gazillion for explaining that. I gave it a shot and I think it works fine now, both for interface based security metadata and implementation based security meta data. I added a property to the service called implementationBasedSecurityMetaData, which defaults to true. If it's set to false, then the service expects the meta data to be keyed on an interface. I'll upload the project with the modified service and test cases next. Thanks again!
        Hide
        Luke Taylor added a comment -

        Do you still think there is a bug? If so, please explain what it is and supply a test case (just against Spring Security code) that demonstrates the problem. If not, I'll close the issue.

        Show
        Luke Taylor added a comment - Do you still think there is a bug? If so, please explain what it is and supply a test case (just against Spring Security code) that demonstrates the problem. If not, I'll close the issue.
        Hide
        Ole Ersoy added a comment -

        Please close it. It works great now - Thanks.

        Show
        Ole Ersoy added a comment - Please close it. It works great now - Thanks.
        Hide
        Ole Ersoy added a comment -

        Actually there is one thing that might be of interest still, and that is that we had to create a "Dummy" MethodSecurityInterceptor, etc in order to create the MethodInvocationPrivilegeEvaluator (See Side Note in description).

        Show
        Ole Ersoy added a comment - Actually there is one thing that might be of interest still, and that is that we had to create a "Dummy" MethodSecurityInterceptor, etc in order to create the MethodInvocationPrivilegeEvaluator (See Side Note in description).
        Hide
        Luke Taylor added a comment -

        You can either inject the interceptor directly into it if you have configured the inteceptor explicitly, autowire it by type or use a BeanPostProcessor to configure it.

        Show
        Luke Taylor added a comment - You can either inject the interceptor directly into it if you have configured the inteceptor explicitly, autowire it by type or use a BeanPostProcessor to configure it.
        Hide
        Ole Ersoy added a comment -

        It's just that the service as is currently chooses which security interceptor to use during the isCallable method invocation, so all the service needs is a "vanilla" MethodInvocationPriviligeEvaluator.

        Show
        Ole Ersoy added a comment - It's just that the service as is currently chooses which security interceptor to use during the isCallable method invocation, so all the service needs is a "vanilla" MethodInvocationPriviligeEvaluator.
        Hide
        Ole Ersoy added a comment -

        That might be a moo point though, if proper framework way to do it is to either use <global-method-security> or manually configure a single method security interceptor that gets applied via aop. If I understand correctly in both of these cases there will only be a single security interceptor, so one might as well just wire it during startup?

        Show
        Ole Ersoy added a comment - That might be a moo point though, if proper framework way to do it is to either use <global-method-security> or manually configure a single method security interceptor that gets applied via aop. If I understand correctly in both of these cases there will only be a single security interceptor, so one might as well just wire it during startup?
        Hide
        Luke Taylor added a comment -

        There isn't really a "proper" way to do it. It depends on your requirements. If a single interceptor is sufficient (and often it will be), then there's no reason why you can't just define the MethodInvocationPrivilegeEvaluator in the app context and inject the interceptor. If for some reason you need more than one interceptor then you can have multiple MethodInvocationPrivilegeEvaluators too.

        Show
        Luke Taylor added a comment - There isn't really a "proper" way to do it. It depends on your requirements. If a single interceptor is sufficient (and often it will be), then there's no reason why you can't just define the MethodInvocationPrivilegeEvaluator in the app context and inject the interceptor. If for some reason you need more than one interceptor then you can have multiple MethodInvocationPrivilegeEvaluators too.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: