SEC-1351: Java Collection Interface are not consistent between classes in API #1595

Closed
spring-issuemaster opened this Issue Jan 4, 2010 · 8 comments

1 participant

@spring-issuemaster

Simon Wong (Migrated from SEC-1351) said:

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. However, this would result API inconsistency for historical classes. One of them is GrantedAuthoritiesContainerImpl.get/setGrantedAuthorities which accept List. 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.

@spring-issuemaster

Luke Taylor said:

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.

@spring-issuemaster

Luke Taylor said:

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.

@spring-issuemaster

Luke Taylor said:

Leaving as is for now.

@spring-issuemaster

Simon Wong said:

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.

@spring-issuemaster

Luke Taylor said:

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

@spring-issuemaster

Luke Taylor said:

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

@spring-issuemaster

Simon Wong said:

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.

@spring-issuemaster

Luke Taylor said:

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.

@spring-issuemaster spring-issuemaster added this to the 3.0.1 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment