Spring Security
  1. Spring Security
  2. SEC-1041

Simplify custom Permission implementations based on AbstractPermission

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4
    • Fix Version/s: 3.0.0 M1
    • Component/s: ACLs
    • Labels:
      None

      Description

      Implementation of custom Permissions should be an easy task. However, if just look at JavaDocs available at Spring site, and you want to extend either AbstractPermission or BasePermission, it is not so easy. You have to call the only constructor taking two parameters: mask and code; but there is no hint in JavaDoc what is mask and what is code (personally I was able to guess what it mask, but I had no idea what is a code). In fact you have to download the source code for Spring Security and only then you can find the the code of AbstractPermission what is the meaning. I believe, you should be able to write you own Permission without looking into the source.
      Also, Javadoc says you cannot use RESERVED_ON or RESERVED_OFF - but looking at JavaDoc you don't know their values!
      So the JavaDoc should be improved.

      Also, I don't understand why I have to pass the code. It is used only for printing the string-representation of Permission in logs or on console, so personally I don't care much (it can simplify debugging, but is not crucial definitively). Only mask is crucial. So why not provide second simplified constructor taking only mask, and using some default "ON" character for active bits (AclFormattingUtils already uses default character '*' for such case in one-arg printBinary() method, we can use it too - BTW. there is also bug in javadoc for this method, the actual character that is being printed is missing)

      1. SEC-1041.patch
        4 kB
        Grzegorz Borkowski

        Activity

        Hide
        Grzegorz Borkowski added a comment -

        I'm attaching the patch with the propositions described earlier:
        1. Fixed javadoc for Permission and AclFormattingUtils classes
        2. Created new field DEFAULT_ON='*' in Permission interface
        3. Added simplified constructor for AbstractPermission and BasePermission, using DEFAULT_ON character for code.

        (I hope I'm not missing some important reason for which you couldn't use such simplified constructor with DEFAULT_ON code character)

        Show
        Grzegorz Borkowski added a comment - I'm attaching the patch with the propositions described earlier: 1. Fixed javadoc for Permission and AclFormattingUtils classes 2. Created new field DEFAULT_ON='*' in Permission interface 3. Added simplified constructor for AbstractPermission and BasePermission, using DEFAULT_ON character for code. (I hope I'm not missing some important reason for which you couldn't use such simplified constructor with DEFAULT_ON code character)
        Hide
        Luke Taylor added a comment -

        Thanks for the patch. I've applied it with the exception that I kept the Permission class as is - i.e. I didn't introduce the extra constant but just added the constructor to AbstractPermission which sets the code to '*'.

        Show
        Luke Taylor added a comment - Thanks for the patch. I've applied it with the exception that I kept the Permission class as is - i.e. I didn't introduce the extra constant but just added the constructor to AbstractPermission which sets the code to '*'.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Grzegorz Borkowski
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: