Spring Security
  1. Spring Security
  2. SEC-1351

Java Collection Interface are not consistent between classes in API

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 3.0.0
    • Fix Version/s: 3.0.1
    • Component/s: None
    • Labels:
      None

      Description

      Most of the Spring Security 3.0 API has been refactored to use Collection<?> rather than native Java Array. e.g. UserDetails.getAuthorities() now returns Collection<GrantedAuthority>. However, this would result API inconsistency for historical classes. One of them is GrantedAuthoritiesContainerImpl.get/setGrantedAuthorities which accept List<GrantedAuthority>. Sometimes, down casting is possible, but sometimes doesn't (For example, grantedAuthoritiesContainerImpl.setGrantedAuthorities(userDetails.getAuthorities()) will not be compiled). The API should be refactored a step more such that consistency could be maintained. Personal speaking, List<?> is preferred to Collection<?> as it supports get(index) function rather than just retrieve element through iteration.

        Activity

        Hide
        Luke Taylor added a comment -

        The choice of Collection over List is mainly intended to allow the use of a Set for the authorities collection. Some implementations use very large numbers of permissions which are mapped to authorities. Using a List or array, the performance of an access-control check degrades as the number of authorities increases. A set can potentially allow a more efficient implementation to be used.

        The GrantedAuthoritiesContainer is typically implemented by the Authentication details object, as a means of obtaining pre-authenticated authorities from an incoming request, so it is unlikely that the collection loaded from a UserDetails would be passed to the setGrantedAuthorities method.

        I'm not sure whether we should attempt to squeeze in an interface change like this to an early 3.0.1 release or not. It is inconsistent, but it is not a serious problem. On the other hand, the impact would also be negligible and it is unlikely to affect many people. Those upgrading from 2 will need to change the interface anyway.

        Show
        Luke Taylor added a comment - The choice of Collection over List is mainly intended to allow the use of a Set for the authorities collection. Some implementations use very large numbers of permissions which are mapped to authorities. Using a List or array, the performance of an access-control check degrades as the number of authorities increases. A set can potentially allow a more efficient implementation to be used. The GrantedAuthoritiesContainer is typically implemented by the Authentication details object, as a means of obtaining pre-authenticated authorities from an incoming request, so it is unlikely that the collection loaded from a UserDetails would be passed to the setGrantedAuthorities method. I'm not sure whether we should attempt to squeeze in an interface change like this to an early 3.0.1 release or not. It is inconsistent, but it is not a serious problem. On the other hand, the impact would also be negligible and it is unlikely to affect many people. Those upgrading from 2 will need to change the interface anyway.
        Hide
        Luke Taylor added a comment -

        Actually, thinking about this some more, I don't really see any pressing reason for interfaces like GrantedAuthoritiesContainer to use a collection instead of a List. The motivation is clear for Authentication (and thus, indirectly, UserDetails) but I don't really see a problem with GrantedAuthoritiesContainer using a List.

        Show
        Luke Taylor added a comment - Actually, thinking about this some more, I don't really see any pressing reason for interfaces like GrantedAuthoritiesContainer to use a collection instead of a List. The motivation is clear for Authentication (and thus, indirectly, UserDetails) but I don't really see a problem with GrantedAuthoritiesContainer using a List.
        Hide
        Luke Taylor added a comment -

        Leaving as is for now.

        Show
        Luke Taylor added a comment - Leaving as is for now.
        Hide
        Simon Wong added a comment -

        Sorry that I am busy on my project development before and late for leaving comment. My scenarios are as follows.

        1. We implement a custom PreAuthenticatedFilter for Sun Microsystems Access Manager (OpenSSO). The OpenSSO will only be used for Single-Sign-On but won't use it's policy filter for authorization.
        2. We implement the UserDetail with JdbcDaoImpl. The user authorities will be stored in database.
        3. We use JdbcDaoImpl to load the user detail and store the user authorities through UserDetails.getAuthorities() in GrantedAuthoritiesContainerImpl.
        4. The authorities information will be used in web frontend as ExtJS which is pure JavaScript and could not make use of Spring Security taglib.

        I guess step 3 is the arguable whether it is the correct approach or not.

        Show
        Simon Wong added a comment - Sorry that I am busy on my project development before and late for leaving comment. My scenarios are as follows. 1. We implement a custom PreAuthenticatedFilter for Sun Microsystems Access Manager (OpenSSO). The OpenSSO will only be used for Single-Sign-On but won't use it's policy filter for authorization. 2. We implement the UserDetail with JdbcDaoImpl. The user authorities will be stored in database. 3. We use JdbcDaoImpl to load the user detail and store the user authorities through UserDetails.getAuthorities() in GrantedAuthoritiesContainerImpl. 4. The authorities information will be used in web frontend as ExtJS which is pure JavaScript and could not make use of Spring Security taglib. I guess step 3 is the arguable whether it is the correct approach or not.
        Hide
        Luke Taylor added a comment -

        That's not really the intended use of GrantedAuthoritiesContainerImpl. You would normally load the authorities through the PreAuthenticationAuthenticationProvider.

        Show
        Luke Taylor added a comment - That's not really the intended use of GrantedAuthoritiesContainerImpl. You would normally load the authorities through the PreAuthenticationAuthenticationProvider.
        Hide
        Luke Taylor added a comment -

        Plus you can always cast or convert the collection to a list if required.

        Show
        Luke Taylor added a comment - Plus you can always cast or convert the collection to a list if required.
        Hide
        Simon Wong added a comment -

        Let me try to update my program to follow the correct use of GrantedAuthoritiesContainerImpl.

        Down casting from Collection to List requires explicit cast and not always works between version (You might implements Collection through Set for performance reason in later version, such that it throws ClassCastException and will breaks the program). Anyway, this issue is just a minor one and always have workaround.

        Thanks.

        Show
        Simon Wong added a comment - Let me try to update my program to follow the correct use of GrantedAuthoritiesContainerImpl. Down casting from Collection to List requires explicit cast and not always works between version (You might implements Collection through Set for performance reason in later version, such that it throws ClassCastException and will breaks the program). Anyway, this issue is just a minor one and always have workaround. Thanks.
        Hide
        Luke Taylor added a comment -

        As I said, you should be loading the authorities from the provider in any case through the injected UserDetailsService. The GrantedAuthoritiesContainer is intended for containing information extracted from the incoming request, not by accessing a database.

        Show
        Luke Taylor added a comment - As I said, you should be loading the authorities from the provider in any case through the injected UserDetailsService. The GrantedAuthoritiesContainer is intended for containing information extracted from the incoming request, not by accessing a database.

          People

          • Assignee:
            Luke Taylor
            Reporter:
            Simon Wong
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: