Jetspeed 2
  1. Jetspeed 2
  2. JS2-475

Proposed changes in portal permissions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1-dev
    • Fix Version/s: 2.1-dev, 2.1
    • Component/s: None
    • Labels:
      None

      Description

      (from email, with added comments)
      I've been looking at the portal permissions and how they are used and think a few things can be simplified and speeded up. If there are no objections to this general direction I will prepare an initial patch.

      1. FolderPermission duplicates the parseActions method from PortalResourcePermission, and in fact calls it's copy again. I think this can be eliminated.

      2. PortalResourcePermission.parseActions seems to have some rather odd code:

      if (token.equals(JetspeedActions.VIEW))
      mask |= JetspeedActions.MASK_VIEW;
      else if (token.equals(JetspeedActions.VIEW) || token.equals(JetspeedActions.RESTORE))
      mask |= JetspeedActions.MASK_VIEW;
      I think this can be simplified.

      3. I may not have found all the constructor uses, but I think that subject should be removed from all the portal permissions. I haven't found any uses of the constructor including a non-null subject (although I might have missed some). In addition to the resulting simplification, I believe the subject has no place in the permissions. The JACC defined permissions for web and ejb do not include a subject. JACC does allow for unchecked permissions, which are difficult to imagine if the permissions involved may include a subject. I think a generally more satisfactory approach is to rely on the policy implementation to determine the subject itself.
      --This requires converting several direct uses of the PermissionManager to AccessController.checkPermission. This is more generatlly consistent with use of Policy to check permissions.

      4. Currently each construction of a portal permission involves string parsing to decipher an actions string. It looks to me as if this can occur hundreds of times for a medium sized portal page. Futhermore, this action string appears to be constructed using ad-hoc string manipulations in AbstractBaseElement.checkPermissions(String actions). Similarly, the constraints implementation seems to do an enormous amount of string comparison to match actions. I think that this can be entirely converted to integer masks with bitwise operations. I'd propose to do this in steps, starting with the permissions and working backwards until I hit the contraints implementation, then converting it.
      – initial patch completely converts permissions to use mask for runtime evaluations. Constraints remain as before.

      5. Some of the constants are duplicated between SecuredResource and JetspeedActions: moved to JetspeedActions only.

      6. There are lots of little bugs like wrongly implemented equals methods in portal permissions

      7. I've fixed the javadoc in the classes in the patch.

      8. This includes the fixes for JS2-472

      I don't really know how to test my changes thoroughly. AFAICT they appear to work with my geronimo/jacc integration (latest version will be posted soon).

      I apologize for the number of white space changes in the diff. I pressed the "reformat" button in idea. The patch is generated with svk and I have sometimes had troubles applying these with patch so I'm also attaching copies of the source files.

      1. JS2-475-additional.diff
        7 kB
        David Jencks
      2. permissions1.diff
        214 kB
        David Jencks
      3. permissions1.jar
        92 kB
        David Jencks
      4. permissions-fragment1.diff
        239 kB
        David Jencks
      5. permissions-fragment3.jar
        121 kB
        David Jencks

        Activity

        Hide
        David Jencks added a comment -

        patch and complete source java files.

        Show
        David Jencks added a comment - patch and complete source java files.
        Hide
        David Jencks added a comment -

        I'm attaching a diff and jar of the combined modifications for JS2-473 and this issue since there are many files modified in both and I did not succeed in producing a diff for the JS2-473 changes alone. If both patches are accepted they can be applied at once with this patch/fileset.

        Show
        David Jencks added a comment - I'm attaching a diff and jar of the combined modifications for JS2-473 and this issue since there are many files modified in both and I did not succeed in producing a diff for the JS2-473 changes alone. If both patches are accepted they can be applied at once with this patch/fileset.
        Hide
        Randy Watler added a comment -

        3. We have proposed removing the Subject from the system and using it only in the PermissionManager if needed. This implies removing all of the "AccessController" code from the PageManager and accessing the PermissionManager instead. +1.

        4. I can take responsibility for the "enormous" number of string operations involved in the constraints implementation. I am not a huge fan of binary bit tests when it comes to readability, but I have to agree that this code is not much better from a readability standpoint. Let me know if you'd like me to fix this or if you are anxious to get to it.

        Show
        Randy Watler added a comment - 3. We have proposed removing the Subject from the system and using it only in the PermissionManager if needed. This implies removing all of the "AccessController" code from the PageManager and accessing the PermissionManager instead. +1. 4. I can take responsibility for the "enormous" number of string operations involved in the constraints implementation. I am not a huge fan of binary bit tests when it comes to readability, but I have to agree that this code is not much better from a readability standpoint. Let me know if you'd like me to fix this or if you are anxious to get to it.
        Hide
        David Jencks added a comment -

        >3. We have proposed removing the Subject from the system and using it only in the PermissionManager if needed. This implies removing all of the "AccessController" code from the PageManager and accessing the PermissionManager instead. +1.

        I'm a bit confused. IIUC the PermissionManager is mostly"hidden" behind the jetspeed Policy implementation. I would think that it would be a good idea to make J2 run against any Policy, in particular a JACC implementation that has the Jetspeed permission installed in it. I think this involves replacing direct reference to the PermissionManager with use of AccessController, which will use the PermissionManager if the Jetspeed policy is installed. This is certainly the direction I have been heading in in the Geronimo integration, which now runs without use of the PermissionManager but with the Jetspeed permissions installed. ( I don't consider this a final solution by any means, but a demonstration that J2 can run with JACC. I think the next step is to push the permissions from the PermissionManager into the JACC PolicyConfiguration)

        >4. I can take responsibility for the "enormous" number of string operations involved in the constraints implementation. I am not a huge fan of binary bit tests when it comes to readability, but I have to agree that this code is not much better from a readability standpoint. Let me know if you'd like me to fix this or if you are anxious to get to it.

        I haven't studied the constraints implementation very much yet. I would prefer to see what the response is to this set of changes before looking at it in detail. I wonder if it would be possible to use the jetspeed permissions within the constraints to decide authorization without dealing with AccessController etc. If possible, this might avoid duplicating the essence of the authorization logic.

        Anyway, thanks for reviewing this patch

        Show
        David Jencks added a comment - >3. We have proposed removing the Subject from the system and using it only in the PermissionManager if needed. This implies removing all of the "AccessController" code from the PageManager and accessing the PermissionManager instead. +1. I'm a bit confused. IIUC the PermissionManager is mostly"hidden" behind the jetspeed Policy implementation. I would think that it would be a good idea to make J2 run against any Policy, in particular a JACC implementation that has the Jetspeed permission installed in it. I think this involves replacing direct reference to the PermissionManager with use of AccessController, which will use the PermissionManager if the Jetspeed policy is installed. This is certainly the direction I have been heading in in the Geronimo integration, which now runs without use of the PermissionManager but with the Jetspeed permissions installed. ( I don't consider this a final solution by any means, but a demonstration that J2 can run with JACC. I think the next step is to push the permissions from the PermissionManager into the JACC PolicyConfiguration) >4. I can take responsibility for the "enormous" number of string operations involved in the constraints implementation. I am not a huge fan of binary bit tests when it comes to readability, but I have to agree that this code is not much better from a readability standpoint. Let me know if you'd like me to fix this or if you are anxious to get to it. I haven't studied the constraints implementation very much yet. I would prefer to see what the response is to this set of changes before looking at it in detail. I wonder if it would be possible to use the jetspeed permissions within the constraints to decide authorization without dealing with AccessController etc. If possible, this might avoid duplicating the essence of the authorization logic. Anyway, thanks for reviewing this patch
        Hide
        David Jencks added a comment -

        Latest version of these changes in JS2-444 geronimo-jetspeed10.zip

        Show
        David Jencks added a comment - Latest version of these changes in JS2-444 geronimo-jetspeed10.zip
        Hide
        David Le Strat added a comment -

        Patch has been committed. I will update JS2-444 to account for the commit.

        Show
        David Le Strat added a comment - Patch has been committed. I will update JS2-444 to account for the commit.
        Hide
        David Jencks added a comment -

        Thanks for comitting the important parts of the patch. I think there are a few places that could use additional cleanup, see this new patch. Most of these remove unused code, but there is also one switch to using a mask rather than a string in a permissions constructor argument.

        I've tried to minimize whitespace changes in this patch.

        Show
        David Jencks added a comment - Thanks for comitting the important parts of the patch. I think there are a few places that could use additional cleanup, see this new patch. Most of these remove unused code, but there is also one switch to using a mask rather than a string in a permissions constructor argument. I've tried to minimize whitespace changes in this patch.
        Hide
        David Le Strat added a comment -

        Applied latest patch (JS2-475-additional.diff).

        Show
        David Le Strat added a comment - Applied latest patch ( JS2-475 -additional.diff).
        Hide
        Ate Douma added a comment -

        Closed again now properly recorded against Fix Version 2.1 as well

        Show
        Ate Douma added a comment - Closed again now properly recorded against Fix Version 2.1 as well

          People

          • Assignee:
            David Le Strat
            Reporter:
            David Jencks
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development