Uploaded image for project: 'Kuali Rice Development'
  1. Kuali Rice Development
  2. KULRICE-7406

Manually cache permissionService.isAuthorized* and roleService.principalHasRole methods

    Details

    • Type: Task
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1.1
    • Component/s: Development
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Application Requirement:
      KFS
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required

      Description

      These caches were introduced in rice 2.1, and then later disabled because of some concerns with derived roles. For performance reasons, these should be manually cached when it is determined that it isn't dealing with a derived role.

        Attachments

          Issue Links

            Activity

            Hide
            kbtaylor Kristina Taylor (Inactive) added a comment -

            Even after the cache was turned off, KC is seeing a different path where the caching is also interfering with derived roles (see KCCOI-276). We are using the methods RoleServiceImpl.getRoleMemberPrincipalIds() and RoleServiceImpl.getRoleMembers(). Everything is working when the cache is completely turned off but when we go with the default caching on Rice 2.1, it is not finding the correct person. Can you please also address this in this issue?

            Show
            kbtaylor Kristina Taylor (Inactive) added a comment - Even after the cache was turned off, KC is seeing a different path where the caching is also interfering with derived roles (see KCCOI-276). We are using the methods RoleServiceImpl.getRoleMemberPrincipalIds() and RoleServiceImpl.getRoleMembers(). Everything is working when the cache is completely turned off but when we go with the default caching on Rice 2.1, it is not finding the correct person. Can you please also address this in this issue?
            Hide
            jjhanso Jeremy Hanson added a comment -

            just did an initial commit for this. It should now manually cache (on the rice side) these methods if it is not dealing with any derived roles.

            What I haven't done yet, is enabling this same thing for the Responsibility service. The responsibility service should work as-is, currently, because the methods dealing with derived roles are not currently being cached.

            Show
            jjhanso Jeremy Hanson added a comment - just did an initial commit for this. It should now manually cache (on the rice side) these methods if it is not dealing with any derived roles. What I haven't done yet, is enabling this same thing for the Responsibility service. The responsibility service should work as-is, currently, because the methods dealing with derived roles are not currently being cached.
            Hide
            jjhanso Jeremy Hanson added a comment -

            attaching initial patch file for 2.0.3 work.

            Show
            jjhanso Jeremy Hanson added a comment - attaching initial patch file for 2.0.3 work.
            Hide
            tschneeb Travis Schneeberger added a comment -

            just out of curiosity, when you do caching for kim manually in the implementation code, how does client-side caching function? Last I knew the client side caching was done by proxying the service interface using a Spring class. I know this has changed slightly but isn't the overall design still in place.

            Another way to think about it is the implementation logic will never execute on the client side so things will be nicely caching on the rice standalone side but clients will still be forced to call out to standalone rice for each service call. Maybe I'm wrong here....

            Show
            tschneeb Travis Schneeberger added a comment - just out of curiosity, when you do caching for kim manually in the implementation code, how does client-side caching function? Last I knew the client side caching was done by proxying the service interface using a Spring class. I know this has changed slightly but isn't the overall design still in place. Another way to think about it is the implementation logic will never execute on the client side so things will be nicely caching on the rice standalone side but clients will still be forced to call out to standalone rice for each service call. Maybe I'm wrong here....
            Hide
            ewestfal Eric Westfall added a comment -

            No, you are not wrong. Your observation is correct here. Though if an application is using "embedded" KIM things would get cached nicely on the client. If they are using thin or remote integration they would not. We decided that for the 2.1.1 patch that was the quickest way to implement it (especially considering that most of the big Rice apps right now use embedded KIM). Otherwise, every time you called a PermissionService.isAuthorized check you would have to first query to find all roles that could potentially have permissions with the given template assigned to them, determine if they are derived or not, and then make the decision on whether or not you can cache based on that. So, obviously that process itself would require a series or remote service calls in order to gather all that information (plus it just seems weird).

            One other thought we had on long-term solution would be if the permission service could just include (as a header on the response) information on the cachability of the result of the authorization check (similar to cache headers in HTTP). That seems like it would work nicely as well because the server-side logic could calculate and determine whether or not that permission was used on any derived roles.

            Show
            ewestfal Eric Westfall added a comment - No, you are not wrong. Your observation is correct here. Though if an application is using "embedded" KIM things would get cached nicely on the client. If they are using thin or remote integration they would not. We decided that for the 2.1.1 patch that was the quickest way to implement it (especially considering that most of the big Rice apps right now use embedded KIM). Otherwise, every time you called a PermissionService.isAuthorized check you would have to first query to find all roles that could potentially have permissions with the given template assigned to them, determine if they are derived or not, and then make the decision on whether or not you can cache based on that. So, obviously that process itself would require a series or remote service calls in order to gather all that information (plus it just seems weird). One other thought we had on long-term solution would be if the permission service could just include (as a header on the response) information on the cachability of the result of the authorization check (similar to cache headers in HTTP). That seems like it would work nicely as well because the server-side logic could calculate and determine whether or not that permission was used on any derived roles.
            Hide
            tschneeb Travis Schneeberger added a comment - - edited

            Hmmm. The header information seems like a nice universal solution although I'm not sure how it fits into server-side caching. I like what we have now where the same interface + annotations are used for the client and server. Makes things simple as long as we take advantage of the condition property on the @Cacheable annotation.

            Show
            tschneeb Travis Schneeberger added a comment - - edited Hmmm. The header information seems like a nice universal solution although I'm not sure how it fits into server-side caching. I like what we have now where the same interface + annotations are used for the client and server. Makes things simple as long as we take advantage of the condition property on the @Cacheable annotation.
            Hide
            ewestfal Eric Westfall added a comment -

            Yeah, but in order to take advantage of the condition property on the @Cacheable annotation, you have to do a bunch of work server-side anyway to determine whether any of the candidate permissions are granted to any derived roles in the first place

            Granted, you could calculate that in a separate remote call and cache it as well, but it seems like it essentially results in client-side caching being intimately dependent on server-side implementation details which smells awfully bad.

            Show
            ewestfal Eric Westfall added a comment - Yeah, but in order to take advantage of the condition property on the @Cacheable annotation, you have to do a bunch of work server-side anyway to determine whether any of the candidate permissions are granted to any derived roles in the first place Granted, you could calculate that in a separate remote call and cache it as well, but it seems like it essentially results in client-side caching being intimately dependent on server-side implementation details which smells awfully bad.
            Hide
            tschneeb Travis Schneeberger added a comment - - edited

            I'm not too sure it is smelly at all from my perspective:

            I propose you do the following:

            1) add RoleService.isDerivedRole(String roleId) and PermissionService.isAssignedToDerivedRole(String permissionId)
            2) make sure these two methods are annotated with @Cacheable
            3) use the condition property to call these methods

            ex:

            @Cacheable(... condition="#(!isDerivedRole(roleId))")

            The other option is:

            1) make a special kim-aware Spring caching proxy
            2) throw the conditional logic in the special proxy (the new proxy would have access to all the inputs of the method with the cache annotations)
            3) use the new kim-aware proxy for the kim module (no idea how to get spring to use this instead of the out of the box one)

            Show
            tschneeb Travis Schneeberger added a comment - - edited I'm not too sure it is smelly at all from my perspective: I propose you do the following: 1) add RoleService.isDerivedRole(String roleId) and PermissionService.isAssignedToDerivedRole(String permissionId) 2) make sure these two methods are annotated with @Cacheable 3) use the condition property to call these methods ex: @Cacheable(... condition="#(!isDerivedRole(roleId))") The other option is: 1) make a special kim-aware Spring caching proxy 2) throw the conditional logic in the special proxy (the new proxy would have access to all the inputs of the method with the cache annotations) 3) use the new kim-aware proxy for the kim module (no idea how to get spring to use this instead of the out of the box one)
            Hide
            gathreya Gayathri Athreya added a comment -

            Works now after overriding the default implementation of dynamicRoleMembership to return true.

            Show
            gathreya Gayathri Athreya added a comment - Works now after overriding the default implementation of dynamicRoleMembership to return true.

              People

              • Assignee:
                jjhanso Jeremy Hanson
                Reporter:
                jjhanso Jeremy Hanson
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: