Details

    • Technical task
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.7.3, 1.8.0
    • core
    • None

    Description

      Similar to other parts we need to prototype support for multiplexing in default permission store

      Attachments

        1. OAK-3777-as-composite.patch
          28 kB
          Alex Deparvu

        Issue Links

          Activity

            Initial prototype implemented with d6a3d262393a. With this change the permissionRoot used by default permission store implementation is a function of mount info for given path i.e. /jcr:system/rep:permissionStore/<mount name>/<workspace name>

            • write - Upon write the permission entry would be created in corresponding mount path
            • read - Upon read the permissions would be collected from all mounts and merged
            /{jcr:primaryType = rep:PermissionStore}
                oak:testMount{jcr:primaryType = rep:PermissionStore}
                  default{jcr:primaryType = rep:PermissionStore}
                    userBob{jcr:primaryType = rep:PermissionStore}
                      -1227964008{rep:accessControlledPath = /testPath/a, jcr:primaryType = rep:PermissionStore}
                        0{rep:isAllow = true, rep:privileges = [1279], jcr:primaryType = rep:Permissions}
                default{jcr:primaryType = rep:PermissionStore}
                  userBob{jcr:primaryType = rep:PermissionStore}
                    -1227964006{rep:accessControlledPath = /testPath/c, jcr:primaryType = rep:PermissionStore}
                      0{rep:isAllow = true, rep:privileges = [1279], jcr:primaryType = rep:Permissions}
            
            chetanm Chetan Mehrotra added a comment - Initial prototype implemented with d6a3d262393a . With this change the permissionRoot used by default permission store implementation is a function of mount info for given path i.e. /jcr:system/rep:permissionStore/<mount name>/<workspace name> write - Upon write the permission entry would be created in corresponding mount path read - Upon read the permissions would be collected from all mounts and merged /{jcr:primaryType = rep:PermissionStore} oak:testMount{jcr:primaryType = rep:PermissionStore} default{jcr:primaryType = rep:PermissionStore} userBob{jcr:primaryType = rep:PermissionStore} -1227964008{rep:accessControlledPath = /testPath/a, jcr:primaryType = rep:PermissionStore} 0{rep:isAllow = true, rep:privileges = [1279], jcr:primaryType = rep:Permissions} default{jcr:primaryType = rep:PermissionStore} userBob{jcr:primaryType = rep:PermissionStore} -1227964006{rep:accessControlledPath = /testPath/c, jcr:primaryType = rep:PermissionStore} 0{rep:isAllow = true, rep:privileges = [1279], jcr:primaryType = rep:Permissions}

            quoting from parent issue:

            as already stated in the discussion during the last Oakathon the whole security
            part in oak is pluggable at runtime and any module may define with it's own
            RepositoryInitializer, Validator and CommitHook implementations. My
            gut feeling therefore is that we shall not twist the current default
            implementations to somehow get it work with this approach but rather make sure
            all multiplexing parts are treated like a separate repo with their own
            configuration(s). Also we must not assume that custom implementations are free
            of 'global' information... so, ultimately I have the feeling that using the
            composite security setup parts might be better suited for this than bending the
            existing code to respect the multiplexing.

            angela Angela Schreiber added a comment - quoting from parent issue: as already stated in the discussion during the last Oakathon the whole security part in oak is pluggable at runtime and any module may define with it's own RepositoryInitializer , Validator and CommitHook implementations. My gut feeling therefore is that we shall not twist the current default implementations to somehow get it work with this approach but rather make sure all multiplexing parts are treated like a separate repo with their own configuration(s). Also we must not assume that custom implementations are free of 'global' information... so, ultimately I have the feeling that using the composite security setup parts might be better suited for this than bending the existing code to respect the multiplexing.
            angela Angela Schreiber added a comment - - edited

            we just had a discussion about the multiplexing authorization models in a meeting. it turned out that I was mistaken about the fact that the physical location of the permission store entries associated with a secondary (private) store which according to the explanation of chetanm is not being written back to the shared (global) nodestore. instead the additional private store keeps it's own 'permission store' and it's only upon read and evaluation that a multiplexing aware permission-entry-reader needs to be aware of the multiplexing.

            so, other authorization models (such as for example oak-authorization-cug can make a conscious decision on whether to support multiplexed setups by using and following the MountProvider API contract.

            with that information at hand my concerns have been addressed and we decided on the following next steps:

            • chetanm will write the documentation on how to write multiplexing aware authorization modules
            • I will then use that docu to adjust oak-authorization-cug, which will allow us to verify that the concept works for more implementations as well (or even possibly refine the concept in case it was needed).
            angela Angela Schreiber added a comment - - edited we just had a discussion about the multiplexing authorization models in a meeting. it turned out that I was mistaken about the fact that the physical location of the permission store entries associated with a secondary (private) store which according to the explanation of chetanm is not being written back to the shared (global) nodestore. instead the additional private store keeps it's own 'permission store' and it's only upon read and evaluation that a multiplexing aware permission-entry-reader needs to be aware of the multiplexing. so, other authorization models (such as for example oak-authorization-cug can make a conscious decision on whether to support multiplexed setups by using and following the MountProvider API contract. with that information at hand my concerns have been addressed and we decided on the following next steps: chetanm will write the documentation on how to write multiplexing aware authorization modules I will then use that docu to adjust oak-authorization-cug , which will allow us to verify that the concept works for more implementations as well (or even possibly refine the concept in case it was needed).

            linking to OAK-4612: as discussed we only want to apply these changes once we verified the feasibility of the proposal with a diff implementation

            angela Angela Schreiber added a comment - linking to OAK-4612 : as discussed we only want to apply these changes once we verified the feasibility of the proposal with a diff implementation
            angela Angela Schreiber added a comment - - edited

            chetanm, do you have any benchmarks illustrating the performance impact of adding multiplexing support to the default permission store? i think that would be really valuable before we put this into production.

            While looking at the patch again I also noticed the following TODO with AuthorizationInitializer:

            + //TODO [multiplex] Add testcase for this

            That brings me to another point: I really would like to have full test coverage for the proposed changes. Would it be possible to add that and provide the coverage numbers on the issue?

            cc: alex.parvulescu

            angela Angela Schreiber added a comment - - edited chetanm , do you have any benchmarks illustrating the performance impact of adding multiplexing support to the default permission store? i think that would be really valuable before we put this into production. While looking at the patch again I also noticed the following TODO with AuthorizationInitializer : + //TODO [multiplex] Add testcase for this That brings me to another point: I really would like to have full test coverage for the proposed changes. Would it be possible to add that and provide the coverage numbers on the issue? cc: alex.parvulescu

            do you have any benchmarks illustrating the performance impact of adding multiplexing support to the default permission store

            As of now no performance number for that. The purpose of this patch was more to prototype possible solution for permission store with basic coverage.

            chetanm Chetan Mehrotra added a comment - do you have any benchmarks illustrating the performance impact of adding multiplexing support to the default permission store As of now no performance number for that. The purpose of this patch was more to prototype possible solution for permission store with basic coverage.
            stillalex Alex Deparvu added a comment -

            attaching a different proposal that explores the idea that a multiplex store is very much like a composite store but with a slightly different behaviour. very much a POC, anchela would love to have your feedback on this idea!

            stillalex Alex Deparvu added a comment - attaching a different proposal that explores the idea that a multiplex store is very much like a composite store but with a slightly different behaviour. very much a POC, anchela would love to have your feedback on this idea!

            alex.parvulescu, sure... will take a close look asap.

            angela Angela Schreiber added a comment - alex.parvulescu , sure... will take a close look asap.

            alex.parvulescu, i very much like the idea with the composite store. but just looking at the patch i don't fully understand how you managed to get away without the changes required to the PermissionHook (see also https://github.com/chetanmeh/jackrabbit-oak/commit/d6a3d262393a98f307b011b09474871c3199c660#diff-0faa66438872f3aa4a33ebd0bde34532). Also i am not totally sure if the or-ing of the evaluation is really providing the correct result... but maybe i am just looking at it from the wrong angle... i would suggest that we start from the testing side and write a comprehensive test-suite for the multiplexing-support within the default permission evaluation... this is currently missing for both approaches, which makes me feel a bit uneasy given the fact that this is such a crucial piece in any Oak repository.

            angela Angela Schreiber added a comment - alex.parvulescu , i very much like the idea with the composite store. but just looking at the patch i don't fully understand how you managed to get away without the changes required to the PermissionHook (see also https://github.com/chetanmeh/jackrabbit-oak/commit/d6a3d262393a98f307b011b09474871c3199c660#diff-0faa66438872f3aa4a33ebd0bde34532 ). Also i am not totally sure if the or-ing of the evaluation is really providing the correct result... but maybe i am just looking at it from the wrong angle... i would suggest that we start from the testing side and write a comprehensive test-suite for the multiplexing-support within the default permission evaluation... this is currently missing for both approaches, which makes me feel a bit uneasy given the fact that this is such a crucial piece in any Oak repository.
            stillalex Alex Deparvu added a comment -

            i would suggest that we start from the testing side and write a comprehensive test-suite for the multiplexing-support within the default permission evaluation... this is currently missing for both approaches, which makes me feel a bit uneasy given the fact that this is such a crucial piece in any Oak repository.

            fully agree. seems this is a very important missing bit

            stillalex Alex Deparvu added a comment - i would suggest that we start from the testing side and write a comprehensive test-suite for the multiplexing-support within the default permission evaluation... this is currently missing for both approaches, which makes me feel a bit uneasy given the fact that this is such a crucial piece in any Oak repository. fully agree. seems this is a very important missing bit
            stillalex Alex Deparvu added a comment -

            I've rebased my current work on top of OAK-6356. The basics of multiplex are covered by the /MultiplexingProviderTest test while the random one MutiplexingProviderRandomTestIT should cover more complex scenarios [0].

            anchela does it make sense to break away the perf tests to a dedicated issue? I can create one and start collecting numbers as a followup.
            I'm not sure if there's anything else we needed to cover here?

            [0] https://github.com/stillalex/jackrabbit-oak/compare/oak-6356...oak-3777

            stillalex Alex Deparvu added a comment - I've rebased my current work on top of OAK-6356 . The basics of multiplex are covered by the /MultiplexingProviderTest test while the random one MutiplexingProviderRandomTestIT should cover more complex scenarios [0] . anchela does it make sense to break away the perf tests to a dedicated issue? I can create one and start collecting numbers as a followup. I'm not sure if there's anything else we needed to cover here? [0] https://github.com/stillalex/jackrabbit-oak/compare/oak-6356...oak-3777

            stillalex, i agree that it makes sense to cover the perf testing in a separate issue and link it to this one.

            angela Angela Schreiber added a comment - stillalex , i agree that it makes sense to cover the perf testing in a separate issue and link it to this one.
            stillalex Alex Deparvu added a comment - fixed with http://svn.apache.org/viewvc?rev=1800129&view=rev

            Bulk close for 1.7.3

            edivad Davide Giannella added a comment - Bulk close for 1.7.3

            People

              stillalex Alex Deparvu
              chetanm Chetan Mehrotra
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: